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

linkage: add --strict flag to detect opportunistic linkage #12454

Merged
merged 1 commit into from
Nov 22, 2021

Conversation

carlocab
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?

There was a previous discussion about making brew linkage --test fail
for unrequested dependencies (#9172). I'm not sure what the outcome of
that was, but it still seems like a good idea to try to help us find
cases of opportunistic linkage as they happen rather than when they
cause CI failures in another PR sometime later.

Let's do this by adding a --strict flag to brew linkage --test. My
intention is for brew linkage --test --strict failures to be warnings
rather than errors in CI, which should mitigate some of the concerns
about doing this that were raised in #9172.

There was a previous discussion about making `brew linkage --test` fail
for unrequested dependencies (Homebrew#9172). I'm not sure what the outcome of
that was, but it still seems like a good idea to try to help us find
cases of opportunistic linkage as they happen rather than when they
cause CI failures in another PR sometime later.

Let's do this by adding a `--strict` flag to `brew linkage --test`. My
intention is for `brew linkage --test --strict` failures to be warnings
rather than errors in CI, which should mitigate some of the concerns
about doing this that were raised in Homebrew#9172.
@BrewTestBot
Copy link
Member

Review period will end on 2021-11-22 at 10:19:40 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Nov 19, 2021
@MikeMcQuaid
Copy link
Member

Makes sense to me.

I think we maybe used to do this (I can't remember) and the common issue was linking to recursive dependencies.

@carlocab
Copy link
Member Author

Linking with recursive dependencies shouldn't cause a brew linkage --test --strict failure, since they should be here:

display_items "Indirect dependencies with linkage", @indirect_deps

If they do then that's a bug we can look at.

@MikeMcQuaid
Copy link
Member

@carlocab Perfect ❤️

@BrewTestBot BrewTestBot added waiting for feedback Merging is blocked until sufficient time has passed for review and removed waiting for feedback Merging is blocked until sufficient time has passed for review labels Nov 19, 2021
@BrewTestBot
Copy link
Member

Review period ended.

@carlocab carlocab merged commit 001bd96 into Homebrew:master Nov 22, 2021
@carlocab carlocab deleted the strict-linkage branch November 22, 2021 12:03
carlocab added a commit to carlocab/homebrew-test-bot that referenced this pull request Nov 22, 2021
carlocab added a commit to carlocab/homebrew-test-bot that referenced this pull request Nov 22, 2021
carlocab added a commit to carlocab/homebrew-test-bot that referenced this pull request Nov 22, 2021
@github-actions github-actions bot added the outdated PR was locked due to age label Dec 23, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 23, 2021
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.

None yet

3 participants