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

cmake: use the cmake snap by default #938

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

soumyaDghosh
Copy link

  • allow using the cmake snap or deb
  • add an option to specify the source for cmake

The snap is set by default because

  1. It's more up-to-date and can run on any base reliably
  2. Debs have a path precedence over the snaps, so, if the cmake snap is shipped with the cmake, the cmake deb is used by default until the PATH is overriden, which is problematic. see here

  • Have you signed the CLA?

- allow using the cmake snap or deb
- add an option to specify the source for cmake
@tigarmo
Copy link
Contributor

tigarmo commented Dec 16, 2024

Hi, thanks for bringing this up. Here are some thoughts:

  • Changing the default to use the snap is a no-go, this will for sure break existing projects.
  • It's unfortunate that the CMake plugin has the build-package hardcoded, but I think the path forward is to actually drop this package in core26+ and let users install whatever CMake they want.
  • For your specific use-case, would it be enough to install the build-snap explicitly on the snapcraft.yaml, and update the PATH to prefer /snap/bin in the build-environment?

@sergiusens
Copy link
Collaborator

I agree with @tigarmo, but one thing we can do is install cmake from build-packages if it fails validation. I would probably only consider this for core26 onwards though

@lengau
Copy link
Contributor

lengau commented Dec 16, 2024

Could we take inspiration from some of the other plugins and not include the cmake build-package if cmake-deps is a dependency of the part? The only existing projects that this would break are ones that have a cmake-deps part and have their cmake-using part run after: [cmake-deps]. GitHub shows 2100 snaps and 17 rocks using the cmake plugin, but none with a cmake-deps part.

@kenvandine
Copy link

Could we take inspiration from some of the other plugins and not include the cmake build-package if cmake-deps is a dependency of the part? The only existing projects that this would break are ones that have a cmake-deps part and have their cmake-using part run after: [cmake-deps]. GitHub shows 2100 snaps and 17 rocks using the cmake plugin, but none with a cmake-deps part.

I like this approach!

@tigarmo
Copy link
Contributor

tigarmo commented Dec 19, 2024

I don't personally like this approach much because if feels like a hacky way to do what @soumyaDghosh really wants to do, which is to use the snap instead of the deb. So they'd add a cmake-deps part which does nothing, just to be able to use a build-snap. In that sense, I think my original suggestion of overriding PATH would be more immediate.

@lengau
Copy link
Contributor

lengau commented Dec 20, 2024

I don't think overriding PATH is necessarily the right way to do it, as it could have unintended side-effects during the build. Remember, we already prepend $CRAFT_STAGE/bin and $CRAFT_PART_INSTALL/bin as well as some other items onto $PATH (something we really need to document), so further manipulating these fields should be done with extreme caution. My recommendation in the short term would be a parts field that looks roughly like:

parts:
  cmake-deps:
    plugin: nil
    build-snaps:
      - cmake
    override-pull: |
      apt-get --yes remove cmake
  neochat:
    plugin: cmake
    after: 
      - cmake-deps
    ...

In the long term, we do need to address this general inconsistency in our plugins. The logic in several of the plugins is that the <plugin name>-deps part is supposed to provide the installation. So a use case installing cmake from the snap would be:

parts:
  cmake-deps:
    plugin: nil
    build-snaps: [cmake]
  neochat:
    after: [cmake-deps]
    plugin: cmake
    source: .
    ...

which isn't really particularly different from what I did above, but without having to manually work around an issue with the plugin.

Currently we have a pretty bad situation regarding dependencies for plugins. Some, like autotools and cmake, simply require a set of packages with no way to override that. Some, like dotnet, require a user to provide the relevant command, which often means users use build-packages, but also has them using build-snaps or doing the install in other ways. A third group, like scons, expects the command(s) to be installed on the system before the first run of the environment validator or for the part to run after a <plugin>-deps part. A fourth group, like npm, has opt-in installation of the dependencies. And finally, a fifth group, like rust and poetry, has opt-out installation of the dependencies.

That's pretty messy for someone whose primary use case is packaging things with craft tools. In the simplest cases, it turns into three groups:

  1. Dependencies are installed by default (autotools, make, cmake, poetry, rust)
  2. Dependencies are not installed by default and the user is expected to install it (dotnet, go, uv)
  3. Dependencies are not installed by default but the plugin offers a way to install them (npm)

However, if we need a custom version of the build tool, we have different groups:

  1. The user can use <plugin>-deps to install said custom version (go, maven, poetry, rust, scons)
  2. The user has to do hackery to replace the current version (autotools, make, cmake)

I think eliminating this last group is a good thing. The root environment validator already ignores a missing or failed command if there's a <plugin>-deps dependency, so to me it seems like a bug that we don't have a general way to tell craft-parts not to install the dependencies.

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.

5 participants