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

Allow python protobuf codegen export to coexist with namespace/host package #21665

Merged
merged 8 commits into from
Nov 20, 2024

Conversation

achrafmam2
Copy link
Contributor

Before this change, if a codegen export clashes with an existing package in the venv the export halts completely. The current change skips the problematic exports. This is sub optimal because not all symbols are exported but better than halting the export.

See #21659

@tgolsson
Copy link
Contributor

Nice! Unfortunately I think the logging here will not be visible, since it would be coming from a child process launched by Pants if I read this code directly -- and thus eaten and not displayed in the common case. I wonder if using shutil.copytree(...., dirs_exist_ok=True) is the simpler solution that solves this permanently?

@achrafmam2
Copy link
Contributor Author

Good idea @tgolsson. Done.

@tgolsson tgolsson added category:bugfix Bug fixes for released features backend: Python Python backend-related issues labels Nov 19, 2024
@tgolsson
Copy link
Contributor

Perfect! Before I unblock CI, I'll have to ask you to add some release notes as well here... otherwise CI will yell at you anyways :)

https://github.com/pantsbuild/pants/blob/main/docs/notes/2.25.x.md

Code expects that after the export codegen_dir is empty.
@achrafmam2
Copy link
Contributor Author

achrafmam2 commented Nov 19, 2024

@tgolsson thanks for walking me through it. I updated the release notes.

Also note that I updated the code, because when I tested locally it failed. There is an extra step that cleans up the codegen_dir which is unfortunately not tested. Maybe once I have more time I can add more coverage.

Note: I was not going to test this locally because I thought the change is small and what could happen the tests are already passing 😅. I forced myself though and it failed. Maybe it's worth to force new authors to do it.

@tgolsson tgolsson changed the title Skip Python Codegen Export of Packages that Already Exist in Venv. Allow python protobuf codegen export to coexist with namespace/host package Nov 19, 2024
@tdyas
Copy link
Contributor

tdyas commented Nov 19, 2024

I forced myself though and it failed. Maybe it's worth to force new authors to do it.

Thanks for testing! (Even small changes can be larger than they seem ...). (And it means my tests did not hit edge cases of which I was unaware when I wrote this code.)

@tgolsson
Copy link
Contributor

tgolsson commented Nov 20, 2024

I've been erring on whether we should backport this to 2.24.x and even possibly 2.23.x, what do you think? This was new in 2.22, right?

@achrafmam2
Copy link
Contributor Author

Backporting to 2.23 seems good to me. The earlier I can use the fix the better. What should I do beside changing the notes?

@tgolsson
Copy link
Contributor

@achrafmam2 We have a bot that does the PRs etc. Asked in #development on slack, I'm not sure how we do the notes now. The notes update from this branch also shouldn't be backported.

@tgolsson tgolsson added this to the 2.23.x milestone Nov 20, 2024
@tgolsson tgolsson merged commit ac4851a into pantsbuild:main Nov 20, 2024
24 checks passed
@WorkerPants
Copy link
Member

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

✔️ 2.23.x

Successfully opened #21676.

✔️ 2.24.x

Successfully opened #21675.


Thanks again for your contributions!

🤖 Beep Boop here's my run link

@achrafmam2 achrafmam2 deleted the export branch November 21, 2024 08:31
tgolsson added a commit that referenced this pull request Nov 26, 2024
…ackage (Cherry-pick of #21665) (#21676)

Before this change, if a codegen export clashes with an existing package
in the venv the export halts completely. The current change skips the
problematic exports. This is sub optimal because not all symbols are
exported but better than halting the export.

See #21659

---------

Co-authored-by: achrafmam2 <[email protected]>
Co-authored-by: Tom Solberg <[email protected]>
tgolsson added a commit that referenced this pull request Nov 26, 2024
…ackage (Cherry-pick of #21665) (#21675)

Before this change, if a codegen export clashes with an existing package
in the venv the export halts completely. The current change skips the
problematic exports. This is sub optimal because not all symbols are
exported but better than halting the export.

See #21659

---------

Co-authored-by: achrafmam2 <[email protected]>
Co-authored-by: Tom Solberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: Python Python backend-related issues category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants