Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Ensure submodule inclusion + Fix the tests #104

Merged
merged 9 commits into from
Jan 14, 2021

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Dec 30, 2020

Description of the change

  • This ensures that the submodules are downloaded before publishing to npm
  • Update temp to fix the tests on Windows

Since #103 is not still merged:

  • Updated the Travis and Appveyor configs just to get CI working for now: Use Node 12 and MSVC 2015 in the CI
  • Regenerated package-lock.json

Benefits

Fixes the Atom bootstrap error:

Details
make: *** No rule to make target 'Release/obj.target/libgit2/deps/libgit2/src/annotated_commit.o', needed by 'Release/obj.target/git2.a'.  Stop.
gyp ERR! build error 
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/usr/lib/node_modules/node-gyp/lib/build.js:194:23)
gyp ERR! stack     at ChildProcess.emit (node:events:376:20)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (node:internal/child_process:277:12)
gyp ERR! System Linux 5.9.11-3-MANJARO
gyp ERR! command "/usr/bin/node" "/usr/lib/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /home/illright/atom/apm/node_modules/atom-package-manager/node_modules/git-utils
gyp ERR! node -v v15.3.0
gyp ERR! node-gyp -v v7.1.0
gyp ERR! not ok

Fixes #69
Closes #105

Verification

The CI and the tests pass:
image

After publishing

Atom and apm should bump their dependency to 5.7.1.

@aminya
Copy link
Contributor Author

aminya commented Dec 30, 2020

@sadick254 @darangi This bug prevents people to be able to bootstrap Atom. Could you merge it as soon as possible? It should be released as 5.7.1

@DeeDeeG

This comment has been minimized.

@aminya
Copy link
Contributor Author

aminya commented Dec 31, 2020

It is fixed in this commit. I would recommend closing the PR to speed up the process.

@aminya aminya changed the title fix: use prepare to init the submodule Ensure submodule inclusion + Fix the tests on Windows Dec 31, 2020
@aminya aminya changed the title Ensure submodule inclusion + Fix the tests on Windows Ensure submodule inclusion + Fix the tests Dec 31, 2020
@aminya aminya force-pushed the submodule-init branch 2 times, most recently from fbd28b1 to 4f2bdc8 Compare January 12, 2021 20:19
@aminya
Copy link
Contributor Author

aminya commented Jan 12, 2021

@sadick254 The issue that this PR fixes is much important than what I see has been merged yesterday. Could you merge this?

Atom cannot be bootstrapped because of this.
Apm cannot be bootstrapped because of this.

FYI, 1 person has left our atom-community organization since two weeks ago. 😞

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Jan 12, 2021

@sadick254 I posted an issue #109 to fully explain the issue, but basically [email protected] is broken and we would request that it be re-uploaded, perhaps as v5.7.1. Because the deps/libgit2 submodule is missing from that version.

This PR (or a similar approach) would make it so the submodule initializes automatically during the npm publish process. I am maybe not 100% sure of every detail of this PR, but the community fork of Atom is currently affected. So some sort of resolution would be much appreciated. The community fork of Atom is trying to pull in this version of git-utils and that is preventing the fork from building. The issue may also interfere with upstream/official Atom some time in the future if not addressed. Sorry to bother you about it, but @aminya is right, it is a somewhat pressing issue IMO.

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Jan 13, 2021

@aminya if we merge atom-community/apm#70 and update our copy of apm then we can work around this by installing apm with exact dependencies (with npm ci).

Eventually having a new version of git-utils published with the submodule included would be much appreciated.

Copy link
Contributor

@sadick254 sadick254 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great.

@sadick254 sadick254 merged commit a265d04 into atom:master Jan 14, 2021
@DeeDeeG
Copy link
Contributor

DeeDeeG commented Jan 14, 2021

Thank you for merging, and for publishing the new release!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node-gyp rebuild fails
3 participants