Skip to content
This repository was archived by the owner on Mar 3, 2023. It is now read-only.

fix/deps: min versions of electron-chromedriver... #23575

Merged
darangi merged 2 commits into from
Feb 7, 2022
Merged

fix/deps: min versions of electron-chromedriver... #23575

darangi merged 2 commits into from
Feb 7, 2022

Conversation

@icecream17
Copy link
Contributor

@icecream17 icecream17 commented Feb 7, 2022 •
edited
Loading

... and electron-snapshot to the current electron version (v11)

Identify the Bug

fixes #23573

Description of the Change

Updates electron-chromedriver and electron-snapshot to the current version (v11) and also adds code in script/lib/check-chromedriver-version to make sure that the versions always match.

This fixes the issue because https://github.com/electron/mksnapshot/releases/tag/v11.0.1 supports arm64 macOS

Alternate Designs

Oh wait, semver has comparison functions like lt and gte, see my suggested changes.

Possible Drawbacks

Oops, I ran npm install npm@6 but forgot to put -g, so I guess the npm dependency is also updated (6.14.8 --> ^6.14.16)

Verification Process

tests pass!

steven nguyen added 2 commits February 7, 2022 14:33
fix/deps: min versions of electron-chromedriver...
9e6302f 
... and electron-snapshot to the current electron version (v11)
sync package-lock
ebcebb2
icecream17
icecream17 commented Feb 7, 2022
View reviewed changes
Copy link
Contributor Author

@icecream17 icecream17 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Alternative design using semver.lt (less than)

script/lib/check-chromedriver-version.js
}

if (!semver.satisfies(chromedriverActualVer, '>=9.0.0')) {
if (!semver.satisfies(chromedriverActualVer, `>=${majorElectronVersion}`)) {
Copy link
Contributor Author

@icecream17 icecream17 Feb 7, 2022

Choose a reason for hiding this comment

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

Suggested change
if (!semver.satisfies(chromedriverActualVer, `>=${majorElectronVersion}`)) {
if (semver.lt(chromedriverActualVer, majorElectronVersion)) {

script/lib/check-chromedriver-version.js
}

if (!semver.satisfies(mksnapshotActualVer, '>=9.0.2')) {
if (!semver.satisfies(mksnapshotActualVer, `>=${majorElectronVersion}`)) {
Copy link
Contributor Author

@icecream17 icecream17 Feb 7, 2022

Choose a reason for hiding this comment

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

Suggested change
if (!semver.satisfies(mksnapshotActualVer, `>=${majorElectronVersion}`)) {
if (semver.lt(mksnapshotActualVer, majorElectronVersion)) {

@darangi
Copy link
Contributor

darangi commented Feb 7, 2022

Thanks for the contribution 🙇🏾‍♂️ @icecream17

@darangi darangi merged commit 8b7ea9a into atom:master Feb 7, 2022
@alines7777
Copy link

alines7777 commented Feb 8, 2022

Thank you very much.

I ran into an issue here as follows with the build script ( presumably as it was attempting to install fs-admin via npm );

   4958 verbose node v17.4.0
   4959 verbose npm  v8.4.1
   4960 error code 1
   4961 error path /Users/alines7777/Git/atom/script/node_modules/fs-admin
   4962 error command failed
   4963 error command sh -c prebuild-install || node-gyp rebuild
 * 4964 error CXX(target) Release/obj.target/fs_admin/src/main.o
 * 4964 error   CXX(target) Release/obj.target/fs_admin/src/fs-admin-darwin.o
   4965 error In file included from ../src/main.cc:1:
   4965 error In file included from ../../nan/nan.h:2884:
 * 4965 error ../../nan/nan_typedarray_contents.h:34:43: error: no member named 'GetContents' in 'v8::ArrayBuffer'
 * 4965 error       data   = static_cast<char*>(buffer->GetContents().Data()) + byte_offset;
 * 4965 error                                   ~~~~~~~~^
   4965 error 1 error generated.
   4965 error make: *** [Release/obj.target/fs_admin/src/main.o] Error 1
   4965 error gyp ERR! build error 
   4965 error gyp ERR! stack Error: `make` failed with exit code: 2
   4965 error gyp ERR! stack     at ChildProcess.onExit (/Users/alines7777/.nvm/versions/node/v17.4.0/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:194:23)
   4965 error gyp ERR! stack     at ChildProcess.emit (node:events:520:28)
   4965 error gyp ERR! stack     at Process.ChildProcess._handle.onexit (node:internal/child_process:291:12)
   4965 error gyp ERR! System Darwin 21.3.0
   4965 error gyp ERR! command "/Users/alines7777/.nvm/versions/node/v17.4.0/bin/node" "/Users/alines7777/.nvm/versions/node/v17.4.0/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
   4965 error gyp ERR! cwd /Users/alines7777/Git/atom/script/node_modules/fs-admin
   4965 error gyp ERR! node -v v17.4.0
   4965 error gyp ERR! node-gyp -v v8.4.1
   4965 error gyp ERR! not ok
   4966 verbose exit 1
   4967 timing npm Completed in 88161ms
   4968 verbose unfinished npm timer reify 1644303141494
   4969 verbose unfinished npm timer reify:build 1644303225111
   4970 verbose unfinished npm timer build 1644303225112
   4971 verbose unfinished npm timer build:deps 1644303225112
   4972 verbose unfinished npm timer build:run:install 1644303225181
   4973 verbose unfinished npm timer build:run:install:node_modules/electron-chromedriver 1644303225181
   4974 verbose unfinished npm timer build:run:install:node_modules/electron-mksnapshot 1644303225186
   4975 verbose unfinished npm timer build:run:install:node_modules/fs-admin 1644303225192
   4976 verbose unfinished npm timer build:run:install:node_modules/leveldown 1644303225197
   4977 verbose code 1

It looks like a shared library component ( of the C/C++ variety from the looks of it ) is failing to compile for the installation of fs-admin, as it seems to be invoking an element [ GetContents().Data() ] that hasn't been declared for the class structure [ buffer ] in line 34 of contents.h.
Frankly, I'm also unsure as to why member function calls [ a property only of classes in C++, rather than data structures in C ] would even be present in a C-type header file. I think maybe someone should contact the developer of fs-admin about this.

@icecream17 icecream17 deleted the update-mksnapshot branch February 8, 2022 13:15
@icecream17
Copy link
Contributor Author

icecream17 commented Feb 24, 2022

The getContents error seems related to this deprecation in oniguruma: atom/node-oniguruma#114, but it doesn't look like the pr directly translates

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Apple macOS 12 ( arm64 ) :: ./script/build : error : script is requesting a distribution of electron/mksnapshot (v9.0.2) that does not exist

3 participants

@icecream17 @darangi @alines7777