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

Add build env values#862

Merged
jasonrudolph merged 1 commit intoatom:masterfrom
Aerijo:build-env
Aug 16, 2019
Merged

Add build env values#862
jasonrudolph merged 1 commit intoatom:masterfrom
Aerijo:build-env

Conversation

@Aerijo
Copy link
Contributor

@Aerijo Aerijo commented Jul 25, 2019

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

Adds the build environment as environment variables. This is more robust than flags, as the process environment will normally be passed along, even if the installation script does not handle and pass the flags.

For example, see node-pty. It defines a custom install script, which does not pass any outer flags to node-gyp. It does use the default process.env however.

Alternate Designs

Could potentially remove the flags now, but I wanted to keep the impact minimal.

Benefits

If the build works, it's probably for the intended Atom version now.

Possible Drawbacks

na.

Verification Process

Successful terminal installation and use in relatively fresh Ubuntu 19.04 VM when installed with apm-nightly install termination. Note a related bug may cause Atom to report the installed version as wrong. If this happens, first try deleting the installed-packages:termination:... entries in localstorage and reloading.

Applicable Issues

I don't know anything about building on Windows, especially how msvs_version is used. While this PR does not harm Windows, it would be good for someone to confirm if more env variables need to be passed to support it.

Edit: It worked on a Windows 10 VM too, where build tools were installed with npm install -g windows-build-tools.

Fixes #860

@Aerijo
Copy link
Contributor Author

Aerijo commented Jul 25, 2019

Build failure appears unrelated

@Aerijo
Copy link
Contributor Author

Aerijo commented Aug 5, 2019

Note the termination repro steps no longer work; it has now got prebuilt binaries for all current versions of Atom.

@vweevers
Copy link

I think this would also help packages that use prebuild-install to download and install prebuilt native modules. For prebuild-install to recognize the runtime as Electron, npm_config_runtime must be set to electron. See prebuild/prebuild-install#106

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.

Native module install errors with latest nightly release

3 participants