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

Don't watch PYTHON_SYS_EXECUTABLE and PATH when unnecessary #1231

Merged
merged 1 commit into from
Oct 12, 2020

Conversation

kngwyu
Copy link
Member

@kngwyu kngwyu commented Oct 12, 2020

Fixes #1229.
I confirmed that this works manually, though it's better if @nikhilmitrax can check this.

@kngwyu
Copy link
Member Author

kngwyu commented Oct 12, 2020

Oops, my first attempt was completely broken...
Please try the force-pushed one.

@nikhilmitrax
Copy link

yeah figured. I will test now and respond back. Unfortunately my test code depends on multiple versions of PyO3, so will backport the patch and test.

@nikhilmitrax
Copy link

nikhilmitrax commented Oct 12, 2020

Seems to be working. ✅

Btw, just my 2c,

It might be more ergonomic to check if PYO3_PYTHON is set at the top of the build script, and setting it to the path of python or python3 to the env variable, and then just simply check PYO3_PYTHON for changes.

Either way, this unblocks me so I'm happy with this.

@kngwyu
Copy link
Member Author

kngwyu commented Oct 12, 2020

It might be more ergonomic to check if PYO3_PYTHON is set at the top of the build script, and setting it to the path of python or python3 to the env variable, and then just simply check PYO3_PYTHON for changes.

But then, if PYO3_PYTHON originates from PATH or PYTHON_SYS_EXECUTABLE, don't we need to check them altogether?

@nikhilmitrax
Copy link

nikhilmitrax commented Oct 12, 2020

I meant something like this:

master...nikhilmitrax:master

(non working code, and the structure definitely needs to be different, but should explain the idea.)

This should achieve the same effect as the original MR, with reduced cargo instructions, and the loop in main()

However, this sort of introduces a magical coupling between find_interpreter() and main(), which I'm not so sure I love. I'll defer to you for what you think is better.

@davidhewitt
Copy link
Member

I'm not entirely convinced that'll work for users that are relying on PATH instead of PYO3_PYTHON, as I don't think cargo will know in that case that it needs to rerun when PATH changes.

As we are happy the existing one implementation works, I'm going to merge this as-is and proceed with releasing it in 0.12.2.

@davidhewitt davidhewitt merged commit 576c6b1 into master Oct 12, 2020
@messense messense deleted the dont-watch-path branch March 18, 2021 02:23
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.

PyO3 always retriggers compilation when used with IDE/Shell combination.
3 participants