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

builder: refactor, remove implicit global state #371

Merged
merged 6 commits into from
Aug 10, 2024
Merged

Conversation

nbraud
Copy link
Collaborator

@nbraud nbraud commented Aug 9, 2024

  • Restructure builder's API, reducing it to

    • a prepare(src: Path, dst: Path) -> Builder context manager
      which sets up an isolated environment and installs the build system's dependencies
    • a Builder abstract class exposing build, metadata, and zipapp methods

    Builder is made abstract, so instances can only be obtained within the managed context, ensuring the build environment was not cleaned up between calls. (Unless one uses RTTI to get the concrete class from a prepared object, or does not use prepare as a context manager...)

  • Add tests for builder ensuring there are no dependencies left on

    • method-call order, nor
    • global state, including:
      • current working directory
      • filesystem state (outside of the IsolatedEnv itself, which is a fresh directory beneath /tmp and source dir)
        in particular the contents of the destination directory should not matter
  • Fix suboptimal use of the builder API, due to API limitations in the rest of bork

    • api.build and api.build_zipapp do not reuse the same Builder, causing isolated environment and wheel to be built twice ;
    • api.release does not have access to the Builder's returned artefact Paths and PackageMetadata, instead performing filesystem-augury
    • pypi.Uploader.upload isn't passed the metadata, and would have to rebuild it again... with no guarantee that the source wasn't modified between bork build and bork release 👿
      That issue already existed, so I won't consider it a blocker for this PR.

    I think I can safely say this will be addressed in follow-up PRs, given that we'll likely end up rewriting Bork's entire API surface, both internal and public.

PS: I just realised I could just squash all those commits together, given that I did a few minor refactors... then rewrote the entire hecking file >_>'

@nbraud nbraud requested a review from duckinator August 9, 2024 21:05
@nbraud
Copy link
Collaborator Author

nbraud commented Aug 9, 2024

Oops I forgot to fix pypi.Uploader.upload (Done, obviously~)

PS: How did mypy not scream about calling a non-existent function? 🙀

@nbraud nbraud force-pushed the remolisher/builder branch 6 times, most recently from 6fbb5f4 to 4294152 Compare August 9, 2024 22:49
@nbraud
Copy link
Collaborator Author

nbraud commented Aug 9, 2024

@duckinator There's something really fucky going on, I keep hitting mistakes which mypy should have caught but didn't

@nbraud nbraud force-pushed the remolisher/builder branch from 0379830 to 5e68330 Compare August 9, 2024 23:07
@nbraud
Copy link
Collaborator Author

nbraud commented Aug 9, 2024

Squashed down all the fixups now that the tests pass

@nbraud nbraud force-pushed the remolisher/builder branch from 6536b39 to 5cf6b37 Compare August 9, 2024 23:30
@nbraud nbraud marked this pull request as ready for review August 9, 2024 23:30
@nbraud
Copy link
Collaborator Author

nbraud commented Aug 9, 2024

I just realised, filesystem-independence isn't really meaningful for builder, since that could only arise from a bug in build.ProjectBuilder, so I guess I'll leave the testing to that for this PR, and start on remolishing the rest of the API 😈

Copy link
Owner

@duckinator duckinator left a comment

Choose a reason for hiding this comment

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

aaaaaa this looks like a fantastic improvement, thank you! 💜

@duckinator duckinator merged commit b82e4d4 into main Aug 10, 2024
20 checks passed
@duckinator duckinator deleted the remolisher/builder branch August 10, 2024 00:02
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