Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: refactor configure args to config_flags #3399

Merged
merged 1 commit into from
Dec 8, 2015
Merged

src: refactor configure args to config_flags #3399

merged 1 commit into from
Dec 8, 2015

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Oct 16, 2015

Main goal here is to get --download-path to configure so we can solve the ICU problems, but while in there I decided that I was sick of the compounding variables we're using to set up the full string so I refactored.

Removed a bunch of variables and rely on %config_flags% alone where possible, also allow for an external %CONFIG_FLAGS% variable to supply additional arguments to configure to match the behaviour of the Makefile. That way we can CONFIG_FLAGS="--download-path=c:\node-icu".

PTAL @nodejs/platform-windows @nodejs/build

@rvagg rvagg mentioned this pull request Oct 16, 2015
Closed
13 tasks
@mscdex mscdex added windows Issues and PRs related to the Windows platform. build Issues and PRs related to build files or the CI. labels Oct 16, 2015
@jbergstroem
Copy link
Member

Perhaps append all old variables to config_flags and pass that as a way of being backwards compatible since we don't know how many consumers we have outside of our own build systems.

@rvagg
Copy link
Member Author

rvagg commented Oct 17, 2015

those variables are all initialized (emptied if they are already set) at the top of the script, not intended to be used from the outside

@jbergstroem
Copy link
Member

I can confirm that config_flags works as intended but as per above question I might not be knowledgable enough to be the only reviewer.

@jbergstroem
Copy link
Member

LGTM

1 similar comment
@jasnell
Copy link
Member

jasnell commented Oct 19, 2015

LGTM

@MylesBorins
Copy link
Contributor

@jasnell LTS?

@rvagg
Copy link
Member Author

rvagg commented Oct 21, 2015

@thealphanerd yes, this will have to be in LTS, if it works.

Warning @nodejs/platform-windows, imma merge this and if I messed something up because you didn't review then imma blame y'all.

orangemocha
orangemocha reviewed Oct 21, 2015
View reviewed changes
vcbuild.bat
if "%i18n_arg%"=="small-icu" set config_flags=%config_flags% --with-intl=small-icu
if "%i18n_arg%"=="intl-none" set config_flags=%config_flags% --with-intl=none

if defined CONFIG_FLAGS set config_flags=%config_flags% %CONFIG_FLAGS%
Copy link
Contributor

Choose a reason for hiding this comment

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

Environment variables are case insensitive on Windows

Copy link
Contributor

Choose a reason for hiding this comment

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

..so if you want to achieve passing additional config flags through an environment var, you would have to name it something different. E.g.: additional_config_flags, or name the other one differently e.g. configure_flags, or _configure_flags

@joaocgreis
Copy link
Member

LGTM

@jbergstroem
Copy link
Member

Me too. LGTM.

@rvagg
Copy link
Member Author

rvagg commented Nov 17, 2015

will land after 5.1.0 cause there's parts of this that can't be tested by CI but could break releases and I don't want to cause yet another release delay

https://ci.nodejs.org/job/node-test-commit/1158/

@rvagg
src: refactor vcbuild configure args creation
b47d823 
remove a bunch of variables and rely on %configure_flags% where
possible, also allow for an external %config_flags% variable to supply
additional arguments to configure to match the behaviour of the Makefile

PR-URL: nodejs#3399
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: João Reis <[email protected]>
@rvagg rvagg merged commit b47d823 into nodejs:master Dec 8, 2015
@rvagg
Copy link
Member Author

rvagg commented Dec 8, 2015

I forgot about this, it was landed on 0.12 and 0.10 already. Landed just now on master @ b47d823

@rvagg rvagg deleted the vcbuild-config-flags branch December 8, 2015 09:43
rvagg added a commit that referenced this pull request Dec 9, 2015
@rvagg
src: refactor vcbuild configure args creation
b08126d 
remove a bunch of variables and rely on %configure_flags% where
possible, also allow for an external %config_flags% variable to supply
additional arguments to configure to match the behaviour of the Makefile

PR-URL: #3399
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: João Reis <[email protected]>
@MylesBorins
Copy link
Contributor

@rvagg this is not patching cleanly on v4.x-staging

Would you be able to make a PR with a working patch

@rvagg rvagg mentioned this pull request Dec 17, 2015
Closed
@MylesBorins
Copy link
Contributor

hey @rvagg another ping regarding this not merging cleanly in 4.x-staging

rvagg added a commit that referenced this pull request Dec 30, 2015
@rvagg
src: refactor vcbuild configure args creation
093bdcb 
remove a bunch of variables and rely on %configure_flags% where
possible, also allow for an external %config_flags% variable to supply
additional arguments to configure to match the behaviour of the Makefile

PR-URL: #3399
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: João Reis <[email protected]>
@rvagg
Copy link
Member Author

rvagg commented Dec 30, 2015

thanks @thealphanerd, merged @ 093bdcb, running a CI for v4.x-staging just to be sure @ https://ci.nodejs.org/job/node-test-commit/1577/, vcbuild.bat seems to have worked just fine on those. The discrepancy was the addition of the vtune stuff which was noise in the patch and I assume we're not backporting to v4.x.

@MylesBorins
Copy link
Contributor

thanks @rvagg

MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
@rvagg
src: refactor vcbuild configure args creation
2b8a32a 
remove a bunch of variables and rely on %configure_flags% where
possible, also allow for an external %config_flags% variable to supply
additional arguments to configure to match the behaviour of the Makefile

PR-URL: #3399
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: João Reis <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
Merged
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@rvagg
src: refactor vcbuild configure args creation
18d2a12 
remove a bunch of variables and rely on %configure_flags% where
possible, also allow for an external %config_flags% variable to supply
additional arguments to configure to match the behaviour of the Makefile

PR-URL: nodejs#3399
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: João Reis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rvagg @jbergstroem @jasnell @MylesBorins @joaocgreis @orangemocha @mscdex