-
-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
gitversion: downgrade, use mono. #53092
Conversation
This makes sense for now. I think it's time for me to try tackle .NET Core since that's what is preventing us building this from source. |
Formula/gitversion.rb
Outdated
uses_from_macos "icu4c" | ||
|
||
def install | ||
libexec.install Dir["*"] | ||
(bin/"gitversion").write <<~EOS | ||
#!/bin/sh | ||
exec "#{libexec}/GitVersion" "$@" | ||
exec "#{Formula["mono"]/opt_bin}/mono" "#{libexec}/GitVersion.exe" "$@" |
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.
exec "#{Formula["mono"]/opt_bin}/mono" "#{libexec}/GitVersion.exe" "$@" | |
exec "#{Formula["mono"].opt_bin}/mono" "#{libexec}/GitVersion.exe" "$@" |
The current version uses a macOS binary which is forbidden in homebrew/core. This will likely fail CI due to being a downgrade but we don't want to intentionally downgrade anyone.
In @GitTools we are discussing how to keep the GitVersion formulae in Homebrew up to date with every release and are wondering what the grounds for this argument is:
@MikeMcQuaid, can you please point to where this is forbidden that also explain why? We are providing a self-contained macOS binary that doesn't require Mono or .NET Core installed, both of which I doubt most macOS users ever will want to install. Isn't it better to provide a dependency-free binary than one which requires a full installation of Mono or .NET Core? |
https://docs.brew.sh/Acceptable-Formulae#we-dont-like-binary-formulae |
It is possible to have them build-only dependencies so that installing from a bottle does not require those dependencies. I tried to get .NET Core in Homebrew/core to help here. Unfortunately .NET Core's source build system is not yet in a state suitable for Homebrew/core but we would consider a system that used the Cask .NET Core (like we do for osxfuse etc.). This shouldn't be too difficult to do. |
This automated check will fail if you add binaries with
Open source binary package managers (e.g. Debian) want to build our own binaries and distribute them to users rather than having upstream do so. Package managers do not like "dependency-free binaries" because they aren't "dependency-free" but instead are "we have bundled all the dependencies into the binary". If there's a critical security vulnerability in Mono: we update Mono and everything depending on it receives the security update. If we have "dependency-free binaries" they all need rebuilt, redistributed and reinstalled before anyone will receive any security updates. Additionally, we (Homebrew) target both macOS and Linux now so binaries built for a single OS are not suitable for us.
Sorry to contract you here @Bo98 but we wouldn't accept formulae in Homebrew/homebrew-core that are dependent on new casks (i.e. those that aren't already installed on our CI machines). |
Apologies - I think I must have misremembered a previous conversation about it from a few months ago. |
@SMillerDev, thanks for the reference. By this argument, I can't see why the GitVersion formulae is permitted to exist at all, since it includes a binary regardless of being self-contained or not. Even in this very PR, a reference is made to
.NET Core is a runtime and needs to be installed just like Mono for the setup the GitVersion formulae has now to work. To work in as a build-only dependency, we would have to replicate building of a self-contained macOS binary within Homebrew somehow. Why is that preferable to GitVersion doing this in our own CI/CD pipeline?
That's a good point.
Another good point. Would moving GitVersion to a cask be the way to go, then? |
Did you read it?
These are cross-platform binaries hence the
Because open source system package managers don't redistribute upstream binaries. We are not alone in this.
No, I don't think so if it can be made to be built from source. If it can be but won't be: a tap may be a good option for users to get the latest versions. |
Thanks for the answers, @MikeMcQuaid.
Yes, but now that you pushed me to read it again, I noticed:
… which in combination with:
… explains everything.
Correct.
Would keeping the current formulae, but changing the dependency to .NET Core be a better way forward, then? |
Great 👍
That seems ideal to me. We'll need to figure out a way to get .NET Core packaged in homebrew-core eventually. |
.NET core is a better option indeed.
What are the options to have the .NET core as dependency before it's available in homebrew-core? |
Thanks for the pointer, @MikeMcQuaid! From my reading, we should be able to fix most of the issues in that PR by submitting a new formulae with a newer version of .NET Core. How do you deal with simultaneous different versions of the same formulae, by the way? I expect that people would like there to exist more than just the latest version of .NET Core in Homebrew; at least the last two major versions should probably be available, if not more. Thoughts? I'll also await @Bo98's input before submitting a PR. |
Hope so!
https://docs.brew.sh/Versions should clarify most of it. The TL;DR for this case would be: it's worth just adding the latest version and we can consider other versions when they are needed. |
I've made some progress on a
Is it possible to use "GitHub checkout" without ending up with a "head only formula"? If it is, how? |
Look for formula defining the |
Is that because submodules are used/needed or some other reason?
Ignore that: it was (almost) four years ago and related to it being a preview tarball which was then patched. |
I caught up on this from dotnet/sdk#11795. What a mess! Yeh, no worries. Just to let you know: this formula may need some hacks to get it working and I'm fine with that (as long as we let upstream know about them so they can change them in future, hopefully). |
The problem with that, is that without a
There are submodules, but while they are not required, they are highly recommended in order to get valuable source information into the build. More importantly, until dotnet/source-build#1695 is released,
Ok.
😅 👍
Thanks, much appreciated. Now that dotnet/source-build#1646 was closed without actually resolving dotnet/source-build#1646 (comment), we don't have a tarball with source information available. This means the build needs to be started with a lot of flags that attempts to circumvent the fact that it is not being done on a Git clone. In other words:
Because of this, the best option seems to be to somehow do a |
It will, look at https://github.com/Homebrew/homebrew-core/blob/master/Formula/cling.rb#L4-L6 |
Cool, fine with me. If there's submodules: it makes sense to use them, too.
This is fine. See @SMillerDev's pointer above on an example formula that does this. |
Ah, I didn't know the |
Yay! I finally have a working build. The final part to nail down is installation. As far as I understand the build instructions, the build should produce a file named something like Given the successfully built tarball, what must happen, is:
I'm not clear on exactly how the build has been performed by running
|
Another question, as I discovered |
Homebrew builds in disposable sandboxes, hence the odd paths you're seeing.
Look in
If you just need the values to construct the path, there's an easier way that leverages on the fact that builds happen in a sandbox, and therefore There Should Be Only One Tarball:
Use the relevant variables here: https://docs.brew.sh/Formula-Cookbook#just-moving-some-files
That happens automagically during pull request CI, so you don't have to worry about it. Since you seem to be new to Homebrew formula building, you should probably spend some time reading https://docs.brew.sh/Formula-Cookbook. |
Don't use that unless absolutely necessary, especially if that causes the build script to build an X11 GUI instead of a native one.
There are no optional dependencies permitted in What you suggest also makes CI testing pointless; when users can point to any .NET version that they installed via other means and say "use that", then scream to high heaven when formulae break, why waste the time and effort? |
Excellent, thanks.
Good point. 👍
I'm now doing the following, which I hope both works and is correct: system "tar", "-xzf", Dir["artifacts/*/*.tar.gz"], "--directory", lib
Brilliant!
I've read it quite thoroughly, but things like
Sorry for being unclear. What I meant was that it would perhaps be a good idea to declare the
I was thinking of allowing formulae dependent on .NET to declare homebrew-core/Formula/couchdb-lucene.rb Line 18 in f31f484
That's a good point, but since it's not possible to declare version dependencies, isn't this the case whenever a formula is upgraded already? Or will a formula upgrade be rejected if any dependent formulae break in the CI tests? |
Thanks to
Also, if I do
So, unfortunately, there isn't just one |
|
Disclaimer: I'm not a Homebrew maintainer. Everything below is based on my own continuing observations of how Homebrew pull requests are resolved, and my own experiences in dealing with software upgrades and breakages.
The current CI testing process for a formula includes testing all dependents of said formula against the new build, and all test failures must be addressed before the upgrade goes through. This is why version bumps of foundational formulae like But once those are addressed, the overall state of "evergreen" Homebrew becomes that much more robust. For instance, Go's recent version bump (#59242) is relatively quiet, likely because the major dependent formulae issues were ironed out in earlier bumps (#55639 and especially #51909). This is likely why the preferred dependency declaration for Java-based formulae (I count 170 at this writing) is This is also why I doubt a |
Thanks for your thorough explanation, @gromgit. It makes perfectly sense. |
We used to do this more in Homebrew but it was a bad idea because we have no way of testing/verifying what formulae work with what versions of dependencies. I'm afraid we won't be going back to this approach.
This, exactly.
I'd suggest that the filename can probably be inferred instead? Something like |
I see. Thanks for explaining.
Yep, I came to the same conclusion by looking at a few other formulae and now have this: tarball = Dir["artifacts/*/Release/dotnet-sdk-#{version}-*.tar.gz"].first
system "tar", "-xzf", tarball, "--directory", lib There's a problem, though. The command fails with:
Is this because I see other formulae extracting first, but they also seem to contain much less than the tarball I'm working with here. It would be awesome if the various lib.install Dir["artifacts/*/Release/dotnet-sdk-#{version}-*.tar.gz"].first, extract: true Until something like that is available, I suppose I will have to extract it into another folder first and then installing from there? Not the most effective way to install this many files, but whatever works, I suppose. |
|
Ah, of course. I'll do that. Thanks! |
|
Thanks for all help! I'm now very close to a working formula.
Point 1 is impossible since writing to Should I just write up instructions for how to do this in a |
I think all of this should be a caveat. We definitely don't want to ask for sudo from users. Additionally, I think an issue should be created to notify upstream of the |
Thanks, @SMillerDev. I noticed, however, that with |
Well done @asbjornu, thanks for all your work on this!
I think we should patch out the
We can use wrapper scripts if needed to set these. We don't want users to have to do so manually.
It won't be
This should be able to be worked around by using |
Thanks for all the help! #60929 is hereby submitted. |
And: it shipped! |
The current version uses a macOS binary which is forbidden in homebrew/core.
This will likely fail CI due to being a downgrade but we don't want to intentionally downgrade anyone.
CC @Bo98 as we talked about this formula.
brew install --build-from-source <formula>
, where<formula>
is the name of the formula you're submitting?brew test <formula>
, where<formula>
is the name of the formula you're submitting?brew audit --strict <formula>
(after doingbrew install <formula>
)?