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.

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

Moving DISTTYPEDIR to CI-Release script for Windows #776

Closed
joaocgreis opened this issue Jun 28, 2017 · 12 comments
Closed

Moving DISTTYPEDIR to CI-Release script for Windows #776

joaocgreis opened this issue Jun 28, 2017 · 12 comments

Comments

@joaocgreis
Copy link
Member

joaocgreis commented Jun 28, 2017

@gibfahn @jbergstroem @rvagg @mhdawson (or anyone else with access to ci-release): can you take a look at https://ci-release.nodejs.org/job/iojs+release/jobConfigHistory/showDiffFiles?timestamp1=2017-06-21_18-30-42&timestamp2=2017-06-28_13-11-55 ? I believe the change is right, but it's better to have a review.

It became necessary after nodejs/node#13900 . I believe it is simpler to have DISTTYPEDIR set in the release script, but have no strong feelings about this.

This is currently under test with a nightly: https://ci-release.nodejs.org/job/iojs+release/1797/ (replaces the one that was automatically started and aborted by me, the job was not fixed yet). EDIT: Nightly was correctly published.

@refack
Copy link
Contributor

refack commented Jun 28, 2017

Ref: nodejs/node#13969
I originally removed it from vcbuild.bat because it was set in a redefined subroutine (which seemed strange), and I couldn't find any reference to it in our code base.
But is there any chance any downstream consumer still uses it (i.e. NVIDIA, Microsoft, or anyone who builds their own node.exe)?

@joaocgreis
Copy link
Member Author

joaocgreis commented Jun 28, 2017

@refack that is possible, any staging server could be used in vcbuild.bat. But I have no idea, never heard of a concrete case of anyone using our build scripts with their own infra.

@refack
Copy link
Contributor

refack commented Jun 28, 2017

http://blog.sec-consult.com/2017/04/application-whitelisting-application.html
NVIDIA build node.exe and sign it with their own certificate 🤦‍♂️

@joaocgreis
Copy link
Member Author

For that they only need to run vcbuild sign in a machine with the certificate installed. Uploading to a staging server goes a bit further 🙂

@refack
Copy link
Contributor

refack commented Jun 28, 2017

AFAICT the nightlies have been built (I looked in the .xz and the new vcbuild.bat is there)

@richardlau
Copy link
Member

But I have no idea, never heard of a concrete case of anyone using our build scripts with their own infra.

IBM does to produce https://developer.ibm.com/node/sdk/.

@joaocgreis
Copy link
Member Author

@richardlau in case you need to keep your infra in sync, my change above only adds set DISTTYPEDIR=%DISTTYPE% before calling vcbuild.bat.

@richardlau
Copy link
Member

@joaocgreis I think DISTTYPEDIR is one of the things we don't use.

cc @gibfahn

@mhdawson
Copy link
Member

@joaocgreis the change in the job likes like the right replacement for what was removed. Having said that I don't quite understand why it was removed from the build script if we need it and other teams that build releases might need it as well.

@gibfahn
Copy link
Member

gibfahn commented Jun 29, 2017

I originally removed it from vcbuild.bat because it was set in a redefined subroutine (which seemed strange), and I couldn't find any reference to it in our code base.

For posterity, according to StackOverflow this will only be used for getnodeversion.

Having said that I don't quite understand why it was removed from the build script if we need it and other teams that build releases might need it as well.

It was removed accidentally, and is re-added in nodejs/node#13969.

@gibfahn
Copy link
Member

gibfahn commented Jul 3, 2017

Just came across nodejs/node#2084, which seems relevant. @joaocgreis I think your change still makes sense in that context.

@joaocgreis
Copy link
Member Author

I believe this can be closed now. Please reopen if there's any issue.

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

No branches or pull requests

5 participants