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

formula: configure git/npm to ignore .brew_home #17116

Merged
merged 1 commit into from
Apr 21, 2024

Conversation

branchvincent
Copy link
Member

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

In core, there's 4 formulae that rm_rf ".brew_home" to ensure git status reports no changes (for some reason, code search is missing helm).

Let's handle this here directly instead with a ~/.gitignore since imo formulae shouldn't even be aware of .brew_home. And because npm respects .gitignore files (as long as there is not also an .npmignore file in the same directory), this removes the need for std_npm_install_args to append to .npmignore.

To demonstrate this shouldn't change anything:

$ cd "$(brew --repo homebrew/core)"

# for git
$ sed -i "" 's/rm_rf ".brew_home"//' Formula/h/helm.rb
$ brew install -s helm && helm version
version.BuildInfo{Version:"v3.14.4", GitCommit:"81c902a123462fd4052bc5e9aa9c513c4c8fc142", GitTreeState:"clean", GoVersion:"go1.22.2"}

# for npm
$ sed -i "" 's/def install/def install\n\t\ttouch ".npmignore"/' Formula/b/bower.rb
$ brew install -s bower && ls "$(brew --prefix bower)"/libexec/lib/node_modules/bower/.brew_home
ls: /opt/homebrew/opt/bower/libexec/lib/node_modules/bower/.brew_home: No such file or directory

Copy link
Member

@ZhongRuoyu ZhongRuoyu left a comment

Choose a reason for hiding this comment

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

Makes sense to me!

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Great idea, thanks again @branchvincent!

@MikeMcQuaid MikeMcQuaid merged commit c9d5b4a into Homebrew:master Apr 21, 2024
25 checks passed
@branchvincent branchvincent deleted the ignore-home branch April 21, 2024 22:25
@@ -2673,6 +2673,8 @@ def inreplace(paths, before = nil, after = nil, audit_result = true, &block) # r
def setup_home(home)
# Don't let bazel write to tmp directories we don't control or clean.
(home/".bazelrc").write "startup --output_user_root=#{home}/_bazel"
# Don't dirty the git tree for git clones.
(home/".gitignore").write "*"
Copy link
Member

Choose a reason for hiding this comment

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

This impacts formulae tests that run git add as it now errors. Not sure if there is a way to restrict to build only. Otherwise will need to update formulae, e.g.

  ==> Testing git-series
  ==> git init
  Initialized empty Git repository in /private/tmp/git-series-test-20240423-56212-o7xmxo/.git/
  ==> git add test
  The following paths are ignored by one of your .gitignore files:
  test
  hint: Use -f if you really want to add them.
  hint: Turn this message off by running
  hint: "git config advice.addIgnoredFile false"
  ==> Testing git-series (again)

Copy link
Member

Choose a reason for hiding this comment

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

@cho-m Yeh, making this build-only with a parameter probably makes sense 👍🏻

Copy link
Member

Choose a reason for hiding this comment

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

Bit odd ~/.gitignore is being read here. Is core.excludesFile set or something?

Copy link
Member

@cho-m cho-m Apr 23, 2024

Choose a reason for hiding this comment

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

Bit odd ~/.gitignore is being read here. Is core.excludesFile set or something?

It is possible this isn't working as intended then.

  • In test, we use HOME=testpath, so ~/.gitignore is often <repo>/.gitignore.
  • In build, we use HOME=buildpath/.brew_home, so it may not actually be used.

Sadly, this is reverse of what we want. EDIT: For npm, this is probably behaving okay in build phase as it looks like they are specifically searching for .gitignore. If we don't care about other git usage, then we can keep installing it as it. I don't know if npm is aware of "modern" git files (e.g. ~/.config/git/ignore)

Copy link
Member

@Bo98 Bo98 Apr 23, 2024

Choose a reason for hiding this comment

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

Ohh right I had in my head for some reason that .brew_home existed in tests too. Makes sense moving this out of setup_home to be in stage only then as it's specific to that surrounding setup.

Copy link
Member

Choose a reason for hiding this comment

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

One thing is whether we only care about npm or want to also deal with all git repos. ~/.gitignore works for npm. ~/.config/git/ignore works for git. However, neither works for both by default.

We would need to write two separate files to support both (in which case we may want to use ~/.npmignore if only used by npm). Alternatively, we would need to write core.excludesFile into ~/.gitconfig or ~/.config/git/config, but the non-standard format (in-house ini variant) makes it less ideal to manually modify.

Copy link
Member

Choose a reason for hiding this comment

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

For now: reverting in #17131. Would appreciate a ✅

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops! Thanks for the quick revert @MikeMcQuaid, sorry didn't realize our test directory structure was different. Trying again with #17141

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants