Closed matthewkeil closed 7 months ago
@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!
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?
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.
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.
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 thels
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)
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.
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.
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?
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 https://github.com/nodejs/unofficial-builds/issues/126
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 https://github.com/nodejs/unofficial-builds/issues/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
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:
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 inall_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.