-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
PS: How did |
6fbb5f4
to
4294152
Compare
@duckinator There's something really fucky going on, I keep hitting mistakes which |
Now the `sdist` and `wheel` distributions are built in the same `IsolatedEnv` but that shouldn't be an issue.
0379830
to
5e68330
Compare
Squashed down all the fixups now that the tests pass |
6536b39
to
5cf6b37
Compare
I just realised, filesystem-independence isn't really meaningful for |
There was a problem hiding this 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! 💜
Restructure
builder
's API, reducing it toprepare(src: Path, dst: Path) -> Builder
context managerwhich sets up an isolated environment and installs the build system's dependencies
Builder
abstract class exposingbuild
,metadata
, andzipapp
methodsBuilder
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 aprepare
d object, or does not useprepare
as a context manager...)Add tests for
builder
ensuring there are no dependencies left onIsolatedEnv
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
andapi.build_zipapp
do not reuse the sameBuilder
, causing isolated environment and wheel to be built twice ;api.release
does not have access to theBuilder
's returned artefactPath
s andPackageMetadata
, instead performing filesystem-augurypypi.Uploader.upload
isn't passed the metadata, and would have to rebuild it again... with no guarantee that the source wasn't modified betweenbork build
andbork 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 >_>'