-
Notifications
You must be signed in to change notification settings - Fork 950
toolchains_llvm: patch to accept cros_sdk as a valid distro #28456
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
Conversation
|
This adds cros_sdk to the list of distributions supporting llvm toolchain builds. |
|
I have also had the same issue with this module trying to run on a distro I care about (NixOS). I found that bumping to the latest version of the module (1.5.0) fixes the problem for me, as the logic to determine valid distributions has been overhauled. |
|
@hcallahan-lowrisc how did you bump the version of the module, is it not determined by Bazel configuration (in which case it should have been updated for everybody)? |
|
Oh I haven’t merged anything yet, just found that fix worked locally. Your PR reminded me. |
I have not tried it, what do I need to change for that? (I am not as bazel literate as I would like to be :) |
|
And one more thing: changing the major version of the package needs to be thoroughly tested, this patch is very light wave and is guaranteed not to affect anybody else |
You can test it just by bumping the version, i.e. this diff and then re-running bazel commands will update the lockfile automatically:
We discussed this informally today, and this module is only used to provide a llvm toolchain for rust bindgen. Updating the module should not change the toolchain, it is the just the infrastructure used to fetch it, so it should be a no-op in practice. I'm happy to confirm that is the case if the change is viable for you too. |
|
This version bump did not work for me. Here's the patch and this is the build error log: |
cros_sdk is the ChromeOS build environment which is a close match for Debian Linux distros. Tested by building opentitantool in ChromeOS SDK environment. Change-Id: Ic50e8cf57b6758fb1053a390e919beeefaabe7be Signed-off-by: Vadim Bendebury <[email protected]>
hcallahan-lowrisc
left a comment
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.
LGTM, I will try to get this fixed upstream so we can go to the latest release and no longer need to carry a patch.
Let me know if you want me to try your fix in ChromeOS chroot build environment. |
I sent you a message on Slack 👍 |
|
The test failures aren't related to the content of this change; both tests pass locally. |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin master
git worktree add -d .worktree/backport-28456-to-master origin/master
cd .worktree/backport-28456-to-master
git switch --create backport-28456-to-master
git cherry-pick -x cbe89842d3c17ee1f0ed92c237cb51243c1d629d |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin master
git worktree add -d .worktree/backport-28456-to-master origin/master
cd .worktree/backport-28456-to-master
git switch --create backport-28456-to-master
git cherry-pick -x cbe89842d3c17ee1f0ed92c237cb51243c1d629d |
cros_sdk is the ChromeOS build environment which is a close match for Debian Linux distros.
Tested by building opentitantool in ChromeOS SDK environment.
Change-Id: Ic50e8cf57b6758fb1053a390e919beeefaabe7be