search

yt-project / yt

Main yt repository
http://yt-project.org
Other
454 stars 272 forks source link

BLD: fix bleeding-edge build script #4898

Closed yut23 closed 1 month ago

yut23 commented 2 months ago

PR Summary

Updates the bleeding-edge CI workflow script to run only pip and adds the numpy API macros to the clib defines.

Fixes #4896.

PR Checklist

yut23 commented 2 months ago

I'm not sure if python setup.py build_ext is necessary anymore, since the build also works fine if I remove the setup.py calls entirely.

neutrinoceros commented 2 months ago

Thanks a lot for following up on this so promptly !

I'm not sure if python setup.py build_ext is necessary anymore, since the build also works fine if I remove the setup.py calls entirely.

You're right, the script is currently mixing up 2 ways to build yt, and pip install already does build + install so setup.py invokes are redundant (and apparently broken) !

There's also a pre-commit lint error (though I don't get why it's poping now).

yut23 commented 2 months ago

Is python setup.py develop still a supported install method? The docs mention it here: https://yt-project.org/docs/dev/visualizing/unstructured_mesh_rendering.html. If it is, then we should probably do something like https://github.com/python-hyper/brotlicffi/pull/92 instead, as develop only runs build_ext, not build_clib.

yut23 commented 2 months ago

yt update also uses setup.py, see update_git() and rebuild_modules() in yt/funcs.py: https://github.com/yt-project/yt/blob/8d58839bc16881e19f5d96c4f46a40db2b26ef0e/yt/funcs.py#L474-L481 https://github.com/yt-project/yt/blob/8d58839bc16881e19f5d96c4f46a40db2b26ef0e/yt/funcs.py#L515-L522

neutrinoceros commented 2 months ago

nice catch. I don't know that we exercise setup.py develop anywhere else and I don't know if it still works as intended. pip install unfortunately does a bit more than what rebuild_modules is supposed to do so I think we should stick to setup.py commands in this function and include the clibs build step your first proposed there. FTR I'm not sure anyone still relies on the yt command line to manage a git repo (or that anyone should), so I would advocate for deprecating them, but that's out of scope for the present PR.

neutrinoceros commented 2 months ago

"Mergeable bot" appears to be stuck. As I don't control it, I'm going to try to close/reopen, hoping this wakes it up.

matthewturk commented 1 month ago

Ah, this seems to be related to where I was running into issues. I believe my current development workflow is broken, as I utilize build_ext -i constantly and have multiple checkouts I switch between. I find it annoying that it's not working, but unless I am able to identify a solution I won't belabor the point here and I'll just try to update my usage.

neutrinoceros commented 1 month ago

It still works if you run build_clib first.

matthewturk commented 1 month ago

Oh, awesome. Thank you :)

On Thu, May 16, 2024 at 12:24 PM Clément Robert @.***> wrote:

It still works if you run build_clib first.

— Reply to this email directly, view it on GitHub https://github.com/yt-project/yt/pull/4898#issuecomment-2115816116, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAVXOY2QOGDFSPMIRGCOFTZCTTUPAVCNFSM6AAAAABHLOZGMWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJVHAYTMMJRGY . You are receiving this because you commented.Message ID: @.***>