-
Notifications
You must be signed in to change notification settings - Fork 94
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
Conversation
@@ -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.""" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
to20
), - 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]>
No description provided.