Skip to content

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.

"**/* *",
"**/*.py",
"**/*.pyc",
"*dist-info/**",
Copy link
Contributor

@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
Contributor

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.

@UebelAndre UebelAndre requested a review from mattem February 24, 2022 21:13
@mattem mattem merged commit 6e0cb65 into bazel-contrib: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