py-spy icon indicating copy to clipboard operation
py-spy copied to clipboard

Remove build.rs

Open jiangliu opened this issue 1 year ago • 4 comments

Afeter commit 5820bf6a7ef56718284add709bc56cc7b8cb6c16, build.rs has no use anymore, so remove.

jiangliu avatar Oct 11 '24 12:10 jiangliu

Where is the unwind feature turned on then?

zanieb avatar Oct 11 '24 13:10 zanieb

Where is the unwind feature turned on then?

When using py-spy as rust library crate, we can enable unwind feature when specifying the dependency. When building py-spy as a binary, we can enable unwind with cargo cmdline options. Unfortunately seems that cargo doesn't support enable features for specific target/arch yet.

BTW, there's no code make use of #[cfg(unwind)] anymore, there have all been converted to #[cfg(feature = "unwind")], so the build.rs becomes useless.

jiangliu avatar Oct 11 '24 14:10 jiangliu

Yeah but that pull request didn't actually update anything to use the unwind feature right? So it just ended up turning it off?

Unfortunately seems that cargo doesn't support enable features for specific target/arch yet.

Yeah that's why it's written as a custom cfg and we'll probably want to revert the feature change and do https://github.com/zanieb/py-spy/pull/9/commits/045846adfefa7df17af52629c28be794ef044776 instead

Alternatively, we need to at least specify the feature per platform during builds. Edit: Looks like @benfred made this change in https://github.com/benfred/py-spy/pull/691.

I don't have strong feelings about which pattern to use. It depends how critical having unwind enabled is, I guess, since people will undoubtably build and test without it enabled accidentally when it's an opt-in feature.

zanieb avatar Oct 12 '24 14:10 zanieb

Yeah but that pull request didn't actually update anything to use the unwind feature right? So it just ended up turning it off?

Unfortunately seems that cargo doesn't support enable features for specific target/arch yet.

Yeah that's why it's written as a custom cfg and we'll probably want to revert the feature change and do zanieb@045846a instead

Alternatively, we need to at least specify the feature per platform during builds. Edit: Looks like @benfred made this change in #691.

I don't have strong feelings about which pattern to use. It depends how critical having unwind enabled is, I guess, since people will undoubtably build and test without it enabled accidentally when it's an opt-in feature.

Hi @zanieb, I have worked out a draft patch to make unwind as default feature, which may help to address your concern:) I would like to make feature unwind work as expect so user can disable unwind when needed.

jiangliu avatar Oct 15 '24 11:10 jiangliu