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

feat: queue recipes #116

Merged
merged 19 commits into from
Apr 6, 2024
Merged

feat: queue recipes #116

merged 19 commits into from
Apr 6, 2024

Conversation

matthewkeil
Copy link
Contributor

@matthewkeil matthewkeil commented Feb 3, 2024

Allows for queueing versions with specific recipes to be built. Defaults to building all recipes if queue item does not have any recipes specified.

queue format is:

v20.1.11
v18.9.0|x64-debub,musl,loong64

Also removes the need to add a recipe to _config.sh. Added _get_recipes.sh that pulls full list from recipes folder for checking recipe input and building all recipes.

To accommodate generating the array I moved fetch-sources folder out of the recipes folder. It can be moved back with recipes though and ill just add a conditional to not include that in all_recipes array. But kind of felt like that should not "live" with recipes. Was a bit confusing to me as a newcomer so guessing others will have the same hurdle when checking out the repo for the first time.

Additionally this PR addresses issue #22 by moving the bundles to dist, building the indexes and shasums after each recipe finishes instead of after all recipes finish.

@matthewkeil matthewkeil marked this pull request as ready for review February 3, 2024 16:19
@matthewkeil
Copy link
Contributor Author

@rvagg and @richardlau I gave this a go. Let me know if all the changes look ok.

In particular I am concerned about _get_recipes.sh. I created that and when I went to PR this I noticed that you added _config.sh with the recipe list already moved over to there. I made that file for the same reason as the list is used in multiple places. Just automated the process a bit so people don't need to add new recipes to the array. Just adding the folder to recipes will do the trick automagically.

I attempted the fix for #22 but am not sure if that part was correct honestly. Haven't dug fully into the checksum or index part of the release process but assumed we would cover it during PR time.

If there is anything that needs to be touched up or backed out feel free to add that to the comments and ill be happy to oblige.

Thanks!

@matthewkeil
Copy link
Contributor Author

matthewkeil commented Feb 3, 2024

There is one other thing I am thinking about touching up in this repo but wanted to ask first. There are a few different syntax versions for variables and sourcing files

ie

. ${__dirname}/_config.sh # no quotes
. "${__dirname}/_get_recipes.sh" # with quotes
source "${__dirname}/_decode_version.sh" # source command

# or

docker run --rm \
# no quotes
  -v ${sourcedir}:/out \
# with quotes
  "${image_tag_pfx}fetch-source" \
# below none use {}
  "$unofficial_release_urlbase" "$disttype" "$customtag" "$datestring" "$commit" "$fullversion" "$source_url" \
# in this line if thislogdir has a space it will fail.  should be quoted
  > ${thislogdir}/fetch-source.log 2>&1 

I started to make these changes but its a lot of little things that clouded this PR so I figured it should be in a separate one.

What do you think?

@rvagg
Copy link
Member

rvagg commented Feb 5, 2024

Drive-by thoughts for now based on the comments here, I haven't really looked at the code yet:

One reason for explicitly listing the recipes is that you can control the order. I think we might still be bailing on all builds if we fail one of them, by ordering them properly we at least get the option of putting the more risky ones last. For now at least that primes the cache for those builds if we trigger a rebuild but in future it should help us prioritise builds to get the most used ones promoted first. None of that is a strongly compelling reason to do it explicitly with the current all-or-nothing approach so my position on this isn't strong, just a preference for being explicit.

I do agree with moving fetch-source out though.

I don't mind you bundling in some consistent quoting to the PR, it's the kind of thing I clean up when I see it too. I'd prefer always quoting when there's doubt fwiw.

@matthewkeil
Copy link
Contributor Author

matthewkeil commented Feb 12, 2024

I've toyed around with a failure queue and having each recipe get attempted for a version so ones that work get built. Before I get to far down the line does that idea seem like it will work for you @rvagg? If so I will also include the script to re-queue the failed builds. Have been busy with the day job this week so will circle back to this later this week to try and finish it up.

README.md Show resolved Hide resolved
```sh
su nodejs # perform the action as the "nodejs" user so as to retain proper queue permissions
cd ~
unofficial-builds/bin/queue-push.sh -v v16.4.0 -r x64-debug -r musl # will only build the `x64-debug` and `musl` recipes for "v16.4.0"
```

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**Important:** Be aware the re-building historical releases will change the digest in the SHASUMS. A consistent digest is required by some consumers of builds, so certain recipes should not be rebuilt. Notably those that are used by the [docker-node](https://github.com/nodejs/docker-node) project, such as `musl`. A change in digest will lead to verification errors downstream. If you are unsure, check with other team members.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a great addition and should definitely be highlighted. I actually moved it above the example so it directly below the "Optionally it is possible..." and before "This can be done by adding..." and the example. Please double check that it feels correct.

bin/build.sh Outdated Show resolved Hide resolved
bin/build.sh Outdated Show resolved Hide resolved
bin/build.sh Outdated Show resolved Hide resolved
bin/prepare-images.sh Outdated Show resolved Hide resolved
bin/prepare-images.sh Outdated Show resolved Hide resolved
bin/queue-push.sh Outdated Show resolved Hide resolved
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

Overall looks good to me; just a few minor suggestions, and we should get input from the docker-node peeps about immediate publish and the lack of ability to prioritise the ones that matter - ideally we'd be able to push out musl first for them and then let everyone else argue about what order the rest should be. That's my main problem with the ls approach vs config file. But it's still better than today, they have to wait till the end of everything regardless of whether it's first.

bin/build.sh Outdated Show resolved Hide resolved
@matthewkeil
Copy link
Contributor Author

Overall looks good to me; just a few minor suggestions, and we should get input from the docker-node peeps about immediate publish and the lack of ability to prioritise the ones that matter - ideally we'd be able to push out musl first for them and then let everyone else argue about what order the rest should be. That's my main problem with the ls approach vs config file. But it's still better than today, they have to wait till the end of everything regardless of whether it's first.

I'm not married to the ls approach and get why it is there now @rvagg. I'm gonna revert to a list of recipes and replace the instructions to put new recipes at the bottom of the list (unless noted during PR review that its a prioritized build)

riscv64 \
loong64 \
"
recipes=(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just converted this to an array to make it easier to work with.

@matthewkeil
Copy link
Contributor Author

matthewkeil commented Feb 14, 2024

Please take another bite at the apple @rvagg. I added back in the recipes (as an array to make easier to work with). I made the changes from the last review and added in the clean-up of quoting and sourcing. I tried to stick to quoting string containing variables and only using curly-brackets where there was concatenation.

@matthewkeil
Copy link
Contributor Author

I still have the the code for queueing failed recipes but will add that as a separate PR. this one is getting pretty big with the quoting cleanup. As a note that commit could definitely be moved to a separate PR if you want.

@matthewkeil
Copy link
Contributor Author

Hi @rvagg. Just wanted to check in with you. I was concerned I made to many changes post review and am worried that may be true now. Do you want me to back out the quoting and sourcing updates and include those in a separate PR to not complicate this PR?

@rvagg rvagg merged commit 9989d24 into nodejs:main Apr 6, 2024
@rvagg
Copy link
Member

rvagg commented Apr 6, 2024

sorry this fell off my radar @matthewkeil, I've been very preoccupied, but now I need this feature! So I'm deploying and will give it a go for #126

@matthewkeil
Copy link
Contributor Author

sorry this fell off my radar @matthewkeil, I've been very preoccupied, but now I need this feature! So I'm deploying and will give it a go for #126

Hi there! Assumed as much. Was hoping you didn't get abducted, but to be fair aliens might be better than the earth at the moment lol!!!

Going everything runs smoothly and will be happy to fix anything that doesn't. Is a bit hard to test that all locally so did my best. If there are any bugs though I'll be happy to hop on it this weekend

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

Successfully merging this pull request may close these issues.

2 participants