build: add build artifact to CI

[gengjiawen]

Lots of user want to help us test or use our new features like quic or other
new exciting feature, but build Node.js from start is not quite easy.

This will help a lot when community want to testing edging WIP features.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[himself65]

suggestion: we could describe this feature in doc, example pull-requests.md

[richardlau]

If you do document this it should include that the artifacts are only available to download after the entire workflow completes (and not when the job completes, so you may need to wait for other platforms to finish before you can download the one you want). Also the artifacts are not permanent (currently expire after 90 days or if manually deleted).

[MylesBorins]

So one thing worth bringing up here... this is a very nice feature but could be abused. This is essentially allowing every PR to create multi-platform binaries that can be downloaded by anyone (if I'm understanding correctly). This does create the possibility that someone could open up a PR with malicious code and we, Node.js, would technically be distributing that to folks. I'm pretty uncomfortable with that TBQH.

[jasnell]

Ah right... good point.

second thoughts...

[Flarna]

What about an extra, manual CI trigger to publish artifacts? This allows collaborators to easily share binaries if needed but no abuseable automatism.

[gengjiawen]

What about an extra, manual CI trigger to publish artifacts ?

Github action doesn't look like has this feature for now.

[addaleax]

@MylesBorins I can see that concern but I feel like the benefits outweigh the risks. Even to just think how much this could speed up bisecting when we have a build available for each commit on master makes me really want to have this.

I think we could rename the file to indicate that it is not an official build? E.g. node-pr-${prid} or node-custom-${commithash}?

[gengjiawen]

Also, it looks like anyone can make a pr add artifact as long as we have github action enabled ? So, enabled it by default won't make much difference ?

[mmarchini]

Also, it looks like anyone can make a pr add artifact as long as we have github action enabled ? So, enabled it by default won't make much difference ?

Changes on Actions coming from forks will only work if the author has commit access, so it's not an issue.

[mmarchini]

Overall I like this :)

Some questions:

• I think GitHub has a space limit for artifacts. How much would be consumed by this?
• Can/should we upload everything from make install instead of just the binary?

[MylesBorins]

@addaleax for the bisecting bit could that not be accomplished with a action only on master as opposed to on every PR?

[addaleax]

@MylesBorins Yes, that’s the case.

[MylesBorins]

@addaleax with that in mind I would like to propose that we scope this PR to only pushes to master, at least for now. That should limit the number of binaries we have to store with github and precludes the security concerns I raised before

[mmarchini]

@MylesBorins does your objection persists here? This would be an amazing feature for us to get feedback from users for specific features which are still in the PR phase, I think it's a good idea to reconsider having it for pushes to the default branch as well as PRs. As a recent example where this would be useful: https://twitter.com/aredridel/status/1305277024446156801. If your objection persist add the Requested Changes to this PR and let's escalate to TSC for decision.

[mmarchini]

@gengjiawen do you mind rebasing this PR?

[MylesBorins]

Fwiw my intent was more a -1 not an objection.

I need to review this again and think it through more extensively, but it was definitely meant to be "let's make sure we think about the risk" not "absolutely don't do this"

I totally trust that you and others working on this would do something thoughtful

[mmarchini]

In terms of risk of malicious code being sent, we might need to wait for a rebase to try some of these, but:

• the binary will be set to the next major with a -pre on the version (indicating it's not official)
  • because of that, installing with npm will be wonky and will need --node-dir to run, therefore folks will probably be more mindful of what they're running
• we can manually delete the artifact if someone sends a PR with malicious code
• the binary/package will not be signed, which is a sign for users that it's not an official

I'm with @addaleax, the benefits outweigh the risks here. Furthermore if we have an incident because of this we can reconsider.

[mmarchini]

I'm huge +1 on this change, just making this comment explicit so we don't land before fixing it:

We should upload everything that is installed with make install, otherwise the user won't have npm, gyp, etc.

[aduh95]

@gengjiawen this would need a rebase.

[github-actions]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[github-actions]

Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.