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

Lock during experimental editable.rebuild #968

Merged
merged 2 commits into from
Jan 14, 2025

Conversation

hauntsaninja
Copy link
Contributor

No description provided.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@henryiii
Copy link
Collaborator

henryiii commented Jan 11, 2025

Can't find the run CI button for the new GitHub interface. Guess I'll switch back to approve, but that's odd that it seems to be missing... (Edit: forgot to check the checks tab, maybe it moved there? Too late now to check.)

@henryiii
Copy link
Collaborator

I'm not sure using a dependency like this will work this way. Scikit-build isn't required to be installed when it rebuilds. So our dependency might not be there. That's why this file doesn't use any dependencies.

@hauntsaninja
Copy link
Contributor Author

I think that's a known issue with the new merge experience.

Hmm, I could make something simple with fcntl and gate to Unix only? Or we could vendor?

@henryiii
Copy link
Collaborator

Unix only would at least be better than the status quo. Vendoring would be hard since we don't even have scikit-build-core present in the user's environment after the editable install.

Pretty sure get_requires_for_build_editable is not what we need, we need to add dependencies to the dependencies itself. Maybe it's okay to do that, since editable installs aren't redistributable?

@hauntsaninja
Copy link
Contributor Author

hauntsaninja commented Jan 11, 2025

Thanks for the quick review. I pushed a Unix only fcntl lock based on filelock's implementation.

@henryiii
Copy link
Collaborator

Thanks!

@henryiii henryiii merged commit ac11716 into scikit-build:main Jan 14, 2025
57 checks passed
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.

None yet

2 participants