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

Generate facts for targets not in nogo scope #4268

Merged
merged 3 commits into from
Feb 18, 2025
Merged

Conversation

fmeum
Copy link
Member

@fmeum fmeum commented Feb 13, 2025

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Nogo analyzers may need facts from their (transitive) dependencies to operate correctly. We thus need to run nogo on all targets, even those not in scope, but should only register the validation action for the targets that are in scope to have them fail the build on nogo findings.

Which issues(s) does this PR fix?

Fixes #4241

Other notes for review

@fmeum fmeum requested review from tyler-french and linzhp February 13, 2025 14:59
@fmeum fmeum force-pushed the 4241-transitive-nogo branch from f833241 to 859724f Compare February 13, 2025 15:00
@fmeum
Copy link
Member Author

fmeum commented Feb 13, 2025

@fionera Could you test this?

@fmeum fmeum force-pushed the 4241-transitive-nogo branch from 859724f to d13aeb2 Compare February 13, 2025 15:01
@fionera
Copy link

fionera commented Feb 14, 2025

@fmeum Things like deprecations are now being correctly detected. Could you maybe also add a "magic package" alias to run nogo including failures it on all external dependencies?

Nogo analyzers may need facts from their (transitive) dependencies to operate correctly. We thus need to run nogo on all targets, even those not in scope, but should only register the validation action for the targets that are in scope to have them fail the build on nogo findings.
@fmeum fmeum force-pushed the 4241-transitive-nogo branch from d13aeb2 to f46f9cc Compare February 18, 2025 07:49
Copy link
Contributor

@linzhp linzhp left a comment

Choose a reason for hiding this comment

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

Tested in Uber by @peng3141 . It works

@fmeum fmeum enabled auto-merge (squash) February 18, 2025 16:38
@fmeum fmeum merged commit 045eb9c into master Feb 18, 2025
4 checks passed
@fmeum fmeum deleted the 4241-transitive-nogo branch February 18, 2025 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow nogo to run against all dependencies
3 participants