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

Fix Pinned Build on Ubuntu 22.04 #833

Merged
merged 1 commit into from
Mar 20, 2023
Merged

Fix Pinned Build on Ubuntu 22.04 #833

merged 1 commit into from
Mar 20, 2023

Conversation

kj4ezj
Copy link
Contributor

@kj4ezj kj4ezj commented Mar 17, 2023

From issue 735, this reverts pull request 799 (Search for LLVM versions 7-11, prefering latest version) to fix the pinned build on Ubuntu 22.04. There are no other changes in this PR besides the revert.

Pull request 799 introduced code that attempts to automagically find the correct version of LLVM for the Leap build on systems that have multiple versions of LLVM installed and available. This code only runs when cmake is version 3.19 or newer. Ubuntu 22.04 ships with CMake version 3.22.1, following the new code path, whereas Ubuntu 20.04 ships with version 3.16.3 that does not follow the new code path. This explains why the pinned build only fails on Ubuntu 22.04.

I have verified the Ubuntu 22.04 pinned build works on this branch.

Credit to @spoonincode for identifying the cause of the bug.

See Also

  1. Pull Request 833 - Fix Pinned Build on Ubuntu 22.04
  2. Pull Request 812 - Pinned Build Script Linter Changes
  3. Pull Request 835 - Change ASCII Art Banner in Build Script
  4. Pull Request 832 - Roll install_deps.sh into pinned_build.sh
  5. Pull Request 847 - Restore libcurl4-openssl-dev
  6. Pull Request 849 - Fix README.md Formatting
  7. Pull Request 870 - Pinned Build Script Backport Omnibus
  8. Pull Request 857 - Simplify Pinned Build Scripts
  9. Pull Request 811 - Pinned Build Script Changes

spoonincode
spoonincode previously approved these changes Mar 17, 2023
Copy link
Member

@spoonincode spoonincode left a comment

Choose a reason for hiding this comment

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

I'm fine with reverting this until a fix is decided on, so we can get the builds operational again asap

@spoonincode spoonincode dismissed their stale review March 17, 2023 15:26

maybe we just go with #834

@ScottBailey
Copy link
Contributor

If #834 fixes the defect, this should be abandoned. #799 does not fail for my non-pinned builds.

Copy link
Contributor

@ScottBailey ScottBailey left a comment

Choose a reason for hiding this comment

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

Evaluate #834 and abandon this if its a solution.

Copy link
Member

@spoonincode spoonincode left a comment

Choose a reason for hiding this comment

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

#834 works but isn't production ready due to it not working with existing dep directories (ugh.. build scripts). #799 is basically causing a release blocker for a nice-to-have.. so I guess I'm back to my "revert it until someone comes up with a shipable fix" approval.

@kj4ezj
Copy link
Contributor Author

kj4ezj commented Mar 17, 2023

Evaluate #834 and abandon this if its a solution.

The base branch is broken and the 4.0 release is blocked. The correct thing to do is to revert the breaking change.

If someone wants to try to re-implement the feature we were attempting to introduce, anyone is welcome to post up another PR. The PR you suggested is a draft, it is not ready for peer review and we do not even know if it would fix the problem if it were. Reverting breaking changes is always the right thing to do because we want the base branch to be in a working state at any given point in time.

@spoonincode
Copy link
Member

fwiw I changed #834 to a draft after Scott made his comment -- after I realized it didn't work in all cases because of how the script works. It does otherwise fix the issue with a fresh depdir

@ScottBailey ScottBailey self-requested a review March 17, 2023 20:31
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.

4 participants