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

snap: modernise snapcraft.yaml #62

Closed
wants to merge 6 commits into from

Conversation

lengau
Copy link
Contributor

@lengau lengau commented Mar 29, 2024

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:

  • Instead of installing software-properties-common and adding the repo + installing additional packages using an override-build script, uses package-repositories to add the repository and build-packages to install the packages.
  • Sets the build environment using the build-environment keyword
  • Uses craftctl instead of snapcraftctl
  • Uses CRAFT_* variables rather than SNAPCRAFT_* variables

The 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.

@rvagg
Copy link
Member

rvagg commented Mar 29, 2024

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?

@lengau
Copy link
Contributor Author

lengau commented Apr 1, 2024

@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:

  1. Fixed a missed change I meant to make
  2. Added a comment to help folks check those minimum versions later
  3. Removed the 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.)

@rvagg rvagg mentioned this pull request May 13, 2024
Closed
7 tasks
@rvagg
Copy link
Member

rvagg commented May 13, 2024

manually merged into main, thanks @lengau !

@rvagg rvagg closed this May 13, 2024
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.

None yet

2 participants