Closed lengau closed 5 months ago
This is great, thanks @lengau! adapting to changes in the config format over time have been a headache when it's not been a primary concern but I know it's been getting stale.
I'll need to do a bit more thorough review, and I probably should check that we're using the right minimum toolchain for current Node. But cursory review this looks positive.
One question though - should we backport some, or all of these changes to other branches that are still active? For each still-active release line of Node.js there's a branch here that gets run in cron and pushed through launchpad. I've occasionally cherry-picked fixes and improvements to them, but otherwise they are mostly as they were when the release line was created. The priority is minimal disruption for people that choose to be on a specific release line, so I don't want to be upgrading the coreXX of those I think for example. Are we going to get into trouble leaving the old build descriptor in place as long as those release lines are active? Or should we be modernising those too?
@rvagg Thanks for your response! I'm going to break it down into bits to make sure I don't lose any threads in what you're saying, as I've been known to miss a few statements sometimes.
I probably should check that we're using the right minimum toolchain for current Node.
I checked this when I was making my PR, mostly as a side-effect of my curiosity about using GCC 10 as opposed to core22's default of GCC 11. The current build docs list GCC/G++ 10.1+ as the minimum version, so I figured that was why you were doing so. The Python and make versions are technically not using the minimum, but I'm not sure it's worth the trouble to install those compared to using what Ubuntu provides.
should we backport some, or all of these changes to other branches that are still active? [...] Are we going to get into trouble leaving the old build descriptor in place as long as those release lines are active? Or should we be modernising those too?
Nah, I think leaving it as-is should be fine. I highly doubt anyone who's using your snapcraft.yaml
to learn will be digging into the older releases, and this PR came purely as a result of me being personally nosy anyway 🙂. Comparing those branches' history and my own changes, I also think any conflicts on a cherry-pick will be self-explanatory and pretty easy to fix, though I think they'll also be unlikely.
Based on your feedback and my own review today, I've made a few more changes:
ubuntu-toolchain-r
test PPA, as it doesn't provide anything necessary for node on core22 (or core24). It looks like it was added for updating to gcc 6 for node 13. (If you'd like I can put it back commented out in case it becomes relevant again.)manually merged into main
, thanks @lengau !
This adjusts the snap to use more modern snapcraft features, which should also help make it easier to do a core24 migration. The build toolchain is unchanged, and the resulting snap should be identical.
Changes:
software-properties-common
and adding the repo + installing additional packages using anoverride-build
script, usespackage-repositories
to add the repository andbuild-packages
to install the packages.build-environment
keywordcraftctl
instead ofsnapcraftctl
CRAFT_*
variables rather thanSNAPCRAFT_*
variablesThe primary motivation behind this is that people often look at big, widely-used projects such as nodejs for help when building their
snapcraft.yaml
files, so using the latest best practices will help encourage others to do so.