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

Add llvm ubuntu 24.04 support #493

Merged
merged 2 commits into from
Aug 15, 2024
Merged

Add llvm ubuntu 24.04 support #493

merged 2 commits into from
Aug 15, 2024

Conversation

gonzalobg
Copy link
Contributor

No description provided.

@@ -337,12 +337,19 @@ def __upstream_package_repos(self):
"""Return the package repositories for the given distro and llvm
version. The development branch repositories are not
versioned and must be handled differently. Currently the
development branch is version 14."""
development branch is version self.__trunk_version."""
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit unrelated, but IMHO it makes sense to update self.__trunk_version to 20 as well now.

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'd like to, instead, propose an alternative, to be pursued in a different PR: we make trunk_version public in the API, and require it when upstream is used. This punts the problem of keeping this up to date to users, while providing users with an immediate way to work around this.

Instead of having to make a change to HPCCM every 6 months about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

we make trunk_version public in the API, and require it when upstream is used. This punts the problem of keeping this up to date to users, while providing users with an immediate way to work around this.

As a user, I'd be a torn about that. The ability to override things is great (but, I think, setting __trunk_version from user code is already a possibility, albeit undocumented). The need to keep track of stuff myself is less great; the magic of HPCCM over bash scripts is that we don't need to keep track of the repositories etc; we want LLVM 15, we get LLVM 15. And the need to specify trunk version even when installing non-trunk is bad.

How about allowing trunk/snapshot as a valid value of version? I see the following use cases:

  • If a user wants a specific version (say, Clang 18) in their container, this is highly likely a released version (or, at least, a pre-release one forked from trunk); in this case, we can go straight to tagged version of the repo.
  • If a user wants the trunk version, chances are they want to specifically track the development branch, not Clang 20 specifically; in this case, specifying trunk would be easiest for all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every user of upstream=True knows that:

  • This feature hasn't worked for almost a year (you are suggesting to bump the version from 18 to 20),
  • Even when regularly patched, it will break one month down the road, making it impractical for people to use.

If someone actually cared enough to fix this,

  • they'll fix it in the LLVM APT package repositories, so that __trunk_version becomes unnecessary, or
  • add significantly more complex logic, to figure out the trunk version dynamically, and make __trunk_version unnecessary.

But until that happens, I think it is better to make it clearer to upstream=True users that they need to specify and maintain the __trunk_upstream version, for their container to work reliably.

Copy link
Contributor

Choose a reason for hiding this comment

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

This feature hasn't worked for almost a year (you are suggesting to bump the version from 18 to 20),

I would not say "not worked"; just "not supported the recent versions".

My concern is that it will be an (apparent) burden on users of stable versions to specify the trunk version when they don't need to. Sure, it will work just fine even if not kept up-to-date, but then the documentation becomes "when setting upstream=True, you should specify both version and trunk_version, where trunk_version is the version of the current development branch of LLVM; the value trunk_version only matters if you're using it; otherwise, any value larger than version would work."

to figure out the trunk version dynamically, and make __trunk_version unnecessary.

For trunk, there are un-versioned packages which also do update-alternatives automatically so that /usr/bin/clang is symlinked to clang-20 or whatever. I don't see __version used for much else in the script.

Copy link
Collaborator

Choose a reason for hiding this comment

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

HPCCM has lots of undocumented "underscore" building block variables like _trunk_version in an attempt to split this difference.

Co-authored-by: Andrey Alekseenko <[email protected]>
@samcmill samcmill merged commit e712954 into NVIDIA:master Aug 15, 2024
15 checks passed
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.

3 participants