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

Exclude dist-info data from pip_repository targets #626

Merged
merged 3 commits into from
Feb 26, 2022

Conversation

UebelAndre
Copy link
Contributor

@UebelAndre UebelAndre commented Feb 23, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A
This is a continuation of

What is the new behavior?

The dist-info directory of a wheel is excluded from the py_library targets as they may contain volatile data.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@UebelAndre
Copy link
Contributor Author

cc @mattem who I think would be a good reviewer.

@@ -147,6 +147,7 @@ def generate_build_file_contents(
"**/* *",
"**/*.py",
"**/*.pyc",
"*dist-info/**",
Copy link
Collaborator

@mattem mattem Feb 24, 2022

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 else in dist-info might be needed, or if anything is. However, we'll want ** as the prefix as some packages contain nested dist-info dirs, internally we patch in "**/*dist-info/RECORD". I'll try and dig out the package we hit this one (that could take me a while, don't block)

Suggested change
"*dist-info/**",
"**/*dist-info/**",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've applied this patch but think it would be good to know the packages. I'm also not necessarily advocating for excluding all of dist-info, I just don't know of a case where it's needed and I know it contains non-deterministic data. Happy to go with something more focused for now. I leave it up to you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll dig them out next week. I think this is likely fine to merge as is. Can always revert or update if it causes issues.

@mattem mattem merged commit 6e0cb65 into bazelbuild:main Feb 26, 2022
@UebelAndre UebelAndre deleted the data branch February 26, 2022 17:50
@UebelAndre
Copy link
Contributor Author

@mattem I've opened a followup at #632 after doing wider testing of these changes.

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.

2 participants