-
Notifications
You must be signed in to change notification settings - Fork 30k
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.
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
build: backport config for new CI infrastructure to v0.10 #3965
Conversation
|
||
==== | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By making additive changes to the license, are we changing it? If so, it is valid? Have we consulted someone familiar with the topic on that matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #3979
For now I'm just copying what's already been done—most of which was done without seeking solid legal advice (and some of it done after ignoring some legal advice ..), now the Foundation has resources we should take it there.
Can we agree to merge this and update from master
down when we have proper legal advice to work with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we agree to merge this and update from master down when we have proper legal advice to work with?
Updating the LICENSE file when we know that we don't know what we're doing seems like a bad idea, especially if it's actually not needed to continue releasing v0.10.x versions. So instead I would suggest not merging these changes, and waiting until we have legal advice to update these files across all relevant branches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@misterdjules agreed, removed from this branch
fe730a2
to
030a1ba
Compare
Updated to match the work merged at #3642, reviews appreciated if anyone is around, @orangemocha, @jbergstroem? |
PR-URL: #3965 Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
PR-URL: #3965 Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
PR-URL: #3965 Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
It seems that the v0.10-staging branch is behind the v0.10 branch. That doesn't seem right. |
if defined nosnapshot set nosnapshot_arg=--without-snapshot | ||
if defined noetw set noetw_arg=--without-etw& set noetw_msi_arg=/p:NoETW=1 | ||
if defined noperfctr set noperfctr_arg=--without-perfctr& set noperfctr_msi_arg=/p:NoPerfCtr=1 | ||
if defined build_release ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike v0.12, we don't set build_release
when build-release
is passed as argument, so this if statement will never be true. See build-release
above. Best to check this after v0.10-staging is rebased on v0.10 though.
When MSBuild invokes rc.exe, it passes NODE_TAG unstringified, but passes it correctly to cl.exe. Hence, this workaround was made to apply only to the resource file. Fixes: nodejs#2963 PR-URL: nodejs#3053 Reviewed-By: Alexis Campailla <orangemocha@nodejs.org> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
23e9b6b
to
8ab062a
Compare
fixed the |
Yes, sorry, I had gotten confused by the fact that the PR was not rebased. LGTM |
CI is also looking good (though the linter should not be running for v0.10): https://ci.nodejs.org/job/node-test-commit/1334/ |
Looks good to me (long form LGTM); didn't have to rebase. Fwiw, I've seen most of this in the 0.12 merge already. |
PR-URL: #3965 Reviewed-By: Alexis Campailla <orangemocha@nodejs.org> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
PR-URL: #3965 Reviewed-By: Alexis Campailla <orangemocha@nodejs.org> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
PR-URL: #3965 Reviewed-By: Alexis Campailla <orangemocha@nodejs.org> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Same as #3642 but for v0.10. Almost the same code but ICU is not here so that all had to be ripped out. I also had to remove the
--download*
stuff from Jenkins when the Node version is^0\.10
.Test build is here: https://nodejs.org/download/nightly/v0.10.41-nightly20151122fe730a2943/
I believe this is ready for a review. In line with my comments in #3642, I think that any of the LICENSE or README stuff that already exists in
master
should be changed there first and backported to these branches rather than these older branches diverging.I'll push a v0.10.41-rc.1 soon, would appreciate if I could get some +1's from tests of the binaries at the above URL though.