Skip to content

build: remove redundant change of working directory#536

Merged
layday merged 2 commits into
pypa:mainfrom
radoering:no-change-working-directory
Nov 21, 2022
Merged

build: remove redundant change of working directory#536
layday merged 2 commits into
pypa:mainfrom
radoering:no-change-working-directory

Conversation

@radoering

Copy link
Copy Markdown
Contributor

As per #535 (comment) changing the working directory seems to be redundant. So let's remove it. (This also makes build thread-safe.)

Closes #535

@henryiii

henryiii commented Nov 20, 2022

Copy link
Copy Markdown
Contributor

Aren't build hooks inherently thread unsafe due to the requirement that they be called in the source directory? You do not pass the source directory into the build hook, it's not part of the interface defined in PEP 517. So the build hook must require the working directory be the source directory. You look up the pyproject.toml by cwd when implementing a hook, as well as whatever it is supposed to build.

A redundant change is fine to remove, but there is always at least one, right?

(I'm thinking of doing multiple builds at the same time - ahh, maybe that's not what you are working on here.)

@layday

layday commented Nov 20, 2022

Copy link
Copy Markdown
Member

A redundant change is fine to remove, but there is always at least one, right?

Do you mean that there is at least one os.chdir in the current thread/process? There isn't, build hooks are executed in a subprocess with the cwd set to the project root.

@layday

layday commented Nov 20, 2022

Copy link
Copy Markdown
Member

Thanks for this. Can you add a simple test to validate that a relative path is made absolute by the ProjectBuilder constructor?

@layday

layday commented Nov 20, 2022

Copy link
Copy Markdown
Member

I'll rebase this on top of #532 as soon as it's unblocked, don't bother fixing the line no :)

@layday

layday commented Nov 20, 2022

Copy link
Copy Markdown
Member

Actually, can you rebase on top of main, @radoering? I don't want to rewrite your history.

@radoering radoering force-pushed the no-change-working-directory branch from 87bf25e to f5f0165 Compare November 21, 2022 05:34
@layday layday merged commit 5760311 into pypa:main Nov 21, 2022
@layday

layday commented Nov 21, 2022

Copy link
Copy Markdown
Member

Thanks!

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.

4 participants