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

chapel 1.26.0 pre-release #98111

Closed

Conversation

tzinsky
Copy link
Contributor

@tzinsky tzinsky commented Mar 30, 2022

This is a pre-release build and we wish to test this new formula against the Homebrew CI. We would like to have this PR given a do not merge label.

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Signed-off-by: Tim Zinsky <tim.zinsky@hpe.com>
@BrewTestBot BrewTestBot added the python Python use is a significant feature of the PR or issue label Mar 30, 2022
Signed-off-by: Tim Zinsky <tim.zinsky@hpe.com>
Signed-off-by: Tim Zinsky <tim.zinsky@hpe.com>
@carlocab carlocab added prerelease-testing Pull request from upstream, testing a pre-release with homebrew dependencies do not merge CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. labels Mar 30, 2022
Adding fix for gcc-5 build failures.

Signed-off-by: Tim Zinsky <tim.zinsky@hpe.com>
Comment on lines 36 to 41
system "echo CHPL_RE2=bundled > chplconfig"
system "echo CHPL_GMP=system >> chplconfig"
system "echo CHPL_LLVM_CONFIG=#{HOMEBREW_PREFIX}/opt/llvm@11/bin/llvm-config >> chplconfig"
system "echo CHPL_LLVM_CONFIG=#{HOMEBREW_PREFIX}/opt/llvm@13/bin/llvm-config >> chplconfig"
# don't try to set CHPL_LLVM_GCC_PREFIX since the llvm@13
# package should be configured to use a reasonable GCC
system 'echo CHPL_LLVM_GCC_PREFIX="none" >> chplconfig'
Copy link
Member

Choose a reason for hiding this comment

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

This could be easier like:

(libexec/"chplconfig").write <<~EOS
  CHPL_RE2=bundled
  CHPL_GMP=system
EOS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for that and I love easier :) Will get that in for our production build...

@@ -16,26 +15,30 @@ class Chapel < Formula
end

depends_on "gmp"
depends_on "llvm@11"
depends_on "llvm"

Choose a reason for hiding this comment

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

@tzinsky : Not related to the current CI failures, but I think this should probably be "llvm@13" so that if/when the default llvm becomes llvm@14 in the future, the formula won't break? (since we don't have llvm 14 support yet).

Choose a reason for hiding this comment

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

(Also, I'm noticing that subsequent commands are hard-coded to llvm@13).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The earlier PR failed audit for this. The error was that the formula should not contain aliases for the most current version. Hence the change to be llvm versus llvm@13

Choose a reason for hiding this comment

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

Oh, interesting!

@SMillerDev : Do you have suggestions for avoiding having this break on the day homebrew's default becomes llvm@14? Since our compiler integrates directly with LLVM, we have to update our compiler sources to become compatible with any new LLVM release (to adjust to the changes to their IR).

Copy link
Member

Choose a reason for hiding this comment

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

The best way is to make sure the test will fail when run with LLVM 14 libraries.

Choose a reason for hiding this comment

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

@Bo98: I suspect that the existing tests will fail when LLVM 14 is the default. Is the idea then that, when that happens and the failure is triggered, a new formula pinning our formula to llvm@13 would need to be submitted?

If so, would the tripping of that failure be something that would be noticed early and automatically during the LLVM upgrade, or would it require a user to hit it in the field and to let us/someone know? And would submitting the revised formula be our responsibility or the developers doing the LLVM upgrade?

Thanks for improving our understanding here!

Copy link
Member

Choose a reason for hiding this comment

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

When a formula is updated, all dependents are tested. So when LLVM is updated to 14, the test of chapel will be run as a part of CI.

The LLVM 14 upgrade won't be merged with failing CI so if it's obvious that migrating to LLVM 13 will fix the test then that's exactly what will be done before merging.

Copy link

Choose a reason for hiding this comment

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

Perfect, thanks for the information and reassurance!

Formula simplification

Signed-off-by: Tim Zinsky <tim.zinsky@hpe.com>
Fix for catalina and llvm

Signed-off-by: Tim Zinsky <tim.zinsky@hpe.com>
Formula syntax change.

Signed-off-by: Tim Zinsky <tim.zinsky@hpe.com>
Audit fixes

Signed-off-by: Tim Zinsky <tim.zinsky@hpe.com>
Another catalina llvm fix

Signed-off-by: Tim Zinsky <tim.zinsky@hpe.com>
@tzinsky tzinsky closed this Mar 31, 2022
@tzinsky tzinsky deleted the bump-chapel-1.26.0-prerelease branch March 31, 2022 20:59
depends_on "python@3.10"
on_macos do
depends_on "llvm" if MacOS.version > :catalina
depends_on "llvm@11" if MacOS.version <= :catalina
Copy link

Choose a reason for hiding this comment

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

@tzinsky , @mppf: Just to make sure I didn't miss anything: We still don't know why llvm@13 didn't work on catalina, is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct. We are able to build outside of homebrew successfully but the homebrew builds were failing.

Copy link

@mppf mppf Apr 1, 2022

Choose a reason for hiding this comment

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

Yes it was the same failure mode as the 3 issues linked below - but for some reason the workaround was not working:

@github-actions github-actions bot added the outdated PR was locked due to age label May 2, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. outdated PR was locked due to age prerelease-testing Pull request from upstream, testing a pre-release with homebrew dependencies python Python use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants