-
-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
recyclarr 7.4.0 (new formula) #198820
recyclarr 7.4.0 (new formula) #198820
Conversation
I wanted to point out some audit issues here, I'm not sure the best way to solve some of these:
Regarding the And regarding the issues with revision/tag: The tag one is confusing because I do specify a tag. Revision is missing because I only want homebrew to check out the tag, I don't know why it has to care about what the specific SHA1 of that tag is. It seems redundant to me, but maybe I'm missing something here. And lastly for the fourth point, I'm not sure what it is saying. This is my first formula so I'm still learning. I went over the cookbook, FAQ, and "how to open a pull request" guides before I opened my PR here. Thanks in advance for any support. |
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. |
end | ||
|
||
test do | ||
assert_match "v#{version}", shell_output("#{bin}/recyclarr --version") |
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.
We need a test that exercises the some of the functionality of the app. Version checks or usage checks (foo --version or foo --help) are not sufficient, as explained in the formula cookbook.
In most cases, a good test would involve running a simple test case: run #{bin}/foo input.txt.
- Then you can check that the output is as expected (with assert_equal or assert_match on the output of shell_output)
- You can also check that an output file was created, if that is expected: assert_predicate temasterath/"output.txt", :exist?
Some advice for specific cases:
- If the formula is a library, compile and run some simple code that links against it. It could be taken from upstream's documentation / source examples.
- If the formula is for a GUI program, try to find some function that runs as command-line only, like a format conversion, reading or displaying a config file, etc.
- If the software cannot function without credentials, a test could be to try to connect with invalid credentials (or without credentials) and confirm that it fails as expected.
- Same if the software requires a virtual machine, docker instance, etc. to be running.
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.
There's no reliable way to exercise the functionality of the application without either 1) setting up a complex suite of external dependencies/services or 2) writing the test against non-deterministic output (i.e. output that can vary depending on changes in external repositories).
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.
can you share the docs how the command is being used?
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.
Docs are on recyclarr.dev, but I am also the author. I suppose I could use the config
subcommand but that won't really exercise more than --version
. Just by doing --version
we're verifying that the application can be set up properly, which is a fairly good smoke test IMO.
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.
Can you run an actual command and assert a failure? That's generally a good test for us.
but I am also the author
We frown on authors submitting their own work unless it is very popular.
https://docs.brew.sh/Acceptable-Formulae#stuff-that-requires-vendored-versions-of-homebrew-formulae:~:text=We%20frown%20on%20authors%20submitting%20their%20own%20work%20unless%20it%20is%20very%20popular.
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.
We frown on authors submitting their own work unless it is very popular
I appreciate you pointing this out to me. I was already aware of this, however, but decided to pursue this anyway since:
- No third party has contributed this in years of waiting (and I did wait)
- I wanted to rely on an exception being made due to the other conditions being met (quoted below).
- be maintained (i.e. the last release wasn’t ages ago, it works without patching on all Homebrew-supported OS versions and has no outstanding, unpatched security vulnerabilities)
- be stable (e.g. not declared “unstable” or “beta” by upstream)
- be known (e.g. GitHub repositories should have >=30 forks, >=30 watchers or >=75 stars)
- be used
- have a homepage
version "7.4.0" | ||
url "https://github.com/recyclarr/recyclarr.git", tag: "v#{version}" |
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.
version "7.4.0" | |
url "https://github.com/recyclarr/recyclarr.git", tag: "v#{version}" | |
url "[tarball](https://github.com/recyclarr/recyclarr/archive/refs/tags/v7.4.0.tar.gz)" | |
sha256 "xxx" |
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.
I cannot use tarball for this because building my application requires a git repository to properly set up versioning (it uses GitVersion). I do not want to try to work around that if possible. I assume because git repositories are supported by formulas that this is an acceptable solution.
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.
can you detailed the logs and dump in here?
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.
I'm not sure what you're asking for. Logs and dump for what?
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.
I think tarball will be ok. Then the version parameter is automatically parsed and can be used in this formula. And checksum is required to prevent any unintended action and we wanna check whether it changes by upstream or not.
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.
@daeho-ro I appreciate the feedback. I just want to be clear on the requirement. Since my project uses GitVersion, it embeds version information into the application by pulling information from the Git repository it is built within. Below is an example snippet from a generated GitVersionInformation.g.cs
file. The structure shown contains information pulled from Git. This cannot be generated if I build from a tarball, because there is no git repository available when a build occurs.
Does this make sense?
static class GitVersionInformation
{
public const string AssemblySemFileVer = "7.4.1.0";
public const string AssemblySemVer = "7.4.1.0";
public const string BranchName = "master";
public const string BuildMetaData = "";
public const string CommitDate = "2024-11-16";
public const string CommitsSinceVersionSource = "4";
public const string EscapedBranchName = "master";
public const string FullBuildMetaData = "Branch.master.Sha.40125ee8950bc75b8544fe18298d1dec325f0a70";
public const string FullSemVer = "7.4.1-dev.4";
public const string InformationalVersion = "7.4.1-dev.4+Branch.master.Sha.40125ee8950bc75b8544fe18298d1dec325f0a70";
public const string Major = "7";
public const string MajorMinorPatch = "7.4.1";
public const string Minor = "4";
public const string Patch = "1";
public const string PreReleaseLabel = "dev";
public const string PreReleaseLabelWithDash = "-dev";
public const string PreReleaseNumber = "4";
public const string PreReleaseTag = "dev.4";
public const string PreReleaseTagWithDash = "-dev.4";
public const string SemVer = "7.4.1-dev.4";
public const string Sha = "40125ee8950bc75b8544fe18298d1dec325f0a70";
public const string ShortSha = "40125ee";
public const string UncommittedChanges = "0";
public const string VersionSourceSha = "4f105f859ad8fd371dbe252062252be07bfd1e17";
public const string WeightedPreReleaseNumber = "55004";
}
--configuration Release | ||
--runtime #{os}-#{arch} | ||
--output #{libexec} | ||
--self-contained true |
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.
why self-contained?
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.
Because I do not want to require dotnet runtime to be installed on systems that install recyclarr. Also it's generally recommended for single-file executable applications to be self contained.
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.
I dont think that is acceptable for us.
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.
Can you explain why please?
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.
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.
docs.brew.sh/Acceptable-Formulae#stuff-that-requires-vendored-versions-of-homebrew-formulae and docs.brew.sh/Acceptable-Formulae#shared-vs-static-libraries
It's not clear to me how the "vendored versions" information directly relates to .NET self contained applications. Could you elaborate?
I'm not sure why Homebrew is being so opinionated about how I build my software. Can you help me understand the objective issue with using a self contained app?
Also from your linked docs it also says:
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.
I'm genuinely interested in working through things but I need to draw a line somewhere. I have yet to see an issue pointed out to me that makes self contained .NET applications not viable.
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.
It's not clear to me how the "vendored versions" information directly relates to .NET self contained applications. Could you elaborate?
By marking the application as self-contained you're vendoring the entire .NET system into the application, which is something that we explicitly disallow.
I'm not sure why Homebrew is being so opinionated about how I build my software. Can you help me understand the objective issue with using a self contained app?
We're not opinionated about how you build the software, but we do have a strong opinion on how we ship software to users in a way that works for them and for us.
I'm genuinely interested in working through things but I need to draw a line somewhere. I have yet to see an issue pointed out to me that makes self contained .NET applications not viable.
It does not fix a problem, it makes one worse. You're trying to fix "what if users don't have .NET", and by doing so you're creating "All my apps ship with their own version of .NET, so now I have 6 versions installed". The whole point of a package manager is for it to manage the package you want, and any it depends on.
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.
For what it's worth (and I don't suspect it's worth much), but "All my apps ship with their own version of .NET, so now I have 6 versions installed" is a false equivalency; The application isn't installing .NET, so it wouldn't really count as multiple side-by-side installations; other .NET applications would not have access to the dependencies shipped with the application.
I really didn't feel that users needed to pull down a .NET runtime separately just to run a single executable application, but I also don't feel super strongly about it. The most important thing is understanding why the requirement exists. I suppose it's just for the ability to address security vulnerabilities in .NET runtime itself without requiring developers to do the upgrade themselves. The same issue will still exist for package references, which are not part of the .NET runtime, but I suppose some is better than none is the policy you're trying to adopt.
I'm happy to make the change. Thanks for clarifying.
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.
Does homebrew support multiple side by side versions of dotnet sdk/runtime? I checked google and found information here that shows people have to use some unofficial workarounds to get this.
I just want to make sure that if I list a dependency on dotnet@8
that this won't conflict with other dotnet versions (e.g. version 9) that users already have installed.
Thank you.
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.
Whatever dependencies your tool has will be managed by brew, which can install multiple versions of dotnet but as a policy only installs supported/major versions
5366f4d
to
48d409d
Compare
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
HOMEBREW_NO_INSTALL_FROM_API=1 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 doingHOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>
)? If this is a new formula, does it passbrew audit --new <formula>
?