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

Explicity allow the use of CMake FetchContent in Solidity formula #172338

Closed

Conversation

r0qs
Copy link
Contributor

@r0qs r0qs commented May 21, 2024

  • 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 HOMEBREW_NO_INSTALL_FROM_API=1 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 HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Since Homebrew/brew#17075, CMake FetchContent is disabled by default . This made our formula to crash: https://github.com/Homebrew/homebrew-core/actions/runs/9175554982/job/25228895627?pr=172324.

This PR fix that by explicitly enabling it back.

See: Homebrew/brew#17187 (comment) for context.

@github-actions github-actions bot added autosquash Automatically squash pull request commits according to Homebrew style. boost Boost use is a significant feature of the PR or issue labels May 21, 2024
@SMillerDev
Copy link
Member

What does it need to download?

@r0qs r0qs force-pushed the enable-fetch-content-solidity-formula branch from 3a61ce0 to 424d534 Compare May 21, 2024 16:30
@r0qs
Copy link
Contributor Author

r0qs commented May 21, 2024

What does it need to download?

Build dependencies like: fmtlib (https://github.com/ethereum/solidity/blob/8a97fa7a1db1ec509221ead6fea6802c684ee887/CMakeLists.txt#L53)

@r0qs r0qs force-pushed the enable-fetch-content-solidity-formula branch from 424d534 to a5d3fb6 Compare May 21, 2024 16:34
@carlocab
Copy link
Member

Copy link
Member

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

It seems this is breaking for the exact reason we introduced this change. Upstream is vendoring formulae. Please adjust the formula to use the Homebrew versions.

@r0qs r0qs force-pushed the enable-fetch-content-solidity-formula branch from a5d3fb6 to 27af624 Compare May 21, 2024 17:02
Copy link
Contributor

Thanks for contributing to Homebrew! 🎉 It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request.

@r0qs r0qs force-pushed the enable-fetch-content-solidity-formula branch 3 times, most recently from 514ffb8 to 49c4068 Compare May 21, 2024 17:18
@r0qs
Copy link
Contributor Author

r0qs commented May 21, 2024

It seems this is breaking for the exact reason we introduced this change. Upstream is vendoring formulae. Please adjust the formula to use the Homebrew versions.

Thanks for the information. But sadly I believe we cannot do that right now. The issue is that the current version of the Solidity compiler installed by this formula (i.e. 0.8.25) uses fetch content unconditionally, so we need to have the option -DHOMEBREW_ALLOW_FETCHCONTENT=ON here.

The mechanism to skip or not system libraries was only added in the recent Solidity release (i.e. 0.8.26, as you can see here: ethereum/solidity@b60ed2b). However the PR that bump the formula version is failing because of the fetch content restriction: #172324).

So I guess we would need to first just add the -DHOMEBREW_ALLOW_FETCHCONTENT=ON here and then we can switch to install the dependencies from homebrew in #172324, since that release also changed one dependency, i.e. we replaced jsoncpp by nlohmann-json. Or we can do all in the PR that bump solidity version, and close this one here, what do you prefer?

Since Homebrew/brew#17075, CMake `FetchContent` is disabled by default.
Which made the Solidity formula to crash due to missing build dependencies: https://github.com/Homebrew/homebrew-core/actions/runs/9175554982/job/25228895627?pr=172324.
This commit fix that by explicitly enabling it back.

See: Homebrew/brew#17187 (comment) for context.
@r0qs r0qs force-pushed the enable-fetch-content-solidity-formula branch from 49c4068 to bfdece6 Compare May 21, 2024 18:07
@SMillerDev
Copy link
Member

The mechanism to skip or not system libraries was only added in the recent Solidity release (i.e. 0.8.26, as you can see here: ethereum/solidity@b60ed2b). However the PR that bump the formula version is failing because of the fetch content restriction: #172324).

Why not just make a PR that bumps the version and uses the Homebrew dependencies in the process?

@r0qs
Copy link
Contributor Author

r0qs commented May 22, 2024

The mechanism to skip or not system libraries was only added in the recent Solidity release (i.e. 0.8.26, as you can see here: ethereum/solidity@b60ed2b). However the PR that bump the formula version is failing because of the fetch content restriction: #172324).

Why not just make a PR that bumps the version and uses the Homebrew dependencies in the process?

Sure, we can try to do that in #172324, so I will be closing this PR.

Does homebrew allow us to use specific versions of those dependencies? Sorry to ask that, but I'm not a macOS user and I don't use homebrew other than in our CI, so I'm not sure if I can specify a formula version with the depends_on, can I?

I'm asking this because we rely on specific version of those dependencies, for instance:

  • fmtlib version 9.1.0
  • nlohmann-json version 3.11.3
  • range-v3 version 0.12.0

For instance, doing the following does not work:

  depends_on "fmt@9.1.0" => :build
Warning: No available formula with the name "fmt@9.1.0" (dependency of solidity).
==> Searching for similarly named formulae...
Error: No formulae found for fmt@9.1.0.

But the problem seems to be that such version is not available in homebrew repo, but if this is the case, how would we be able to use homebrew dependencies if we depend on some previous versions?

@r0qs r0qs closed this May 22, 2024
@SMillerDev
Copy link
Member

No, the reasoning why that is unsupported is included in the docs for it: https://docs.brew.sh/Acceptable-Formulae#stuff-that-requires-vendored-versions-of-homebrew-formulae

@matheusaaguiar
Copy link
Contributor

matheusaaguiar commented May 23, 2024

Isn't that too restrictive? I mean, sometimes it is difficult and time consuming to update every dependency every time there is a new version. Sometimes, there is third-party functionality that was deprecated and cannot be readily replaced/re-worked. Other times, we just want to use more stable/audited software and that means fixing a specific version.
Why not implement what was suggested here by CMake maintainer?
Also, from the docs, Increasingly, though: this can be (too) hard. Homebrew’s primary mission is to be useful rather than ideologically pure. If we cannot package something without using vendored upstream versions: so be it; better to have packaged software in Homebrew than not.

Don't get me wrong, not trying to be confrontational, just raising some questions/arguments.
Thanks.

@carlocab
Copy link
Member

Why not implement what was suggested here by CMake maintainer?

We have implemented it. See:

Homebrew/brew#17075 (comment)
Homebrew/brew#17310

@matheusaaguiar
Copy link
Contributor

We have implemented it. See:

Homebrew/brew#17075 (comment) Homebrew/brew#17310

Ah, my bad, I didn't see that one.

@github-actions github-actions bot added the outdated PR was locked due to age label Jun 24, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosquash Automatically squash pull request commits according to Homebrew style. boost Boost use is a significant feature of the PR or issue outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants