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

Entrypoints #1479

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Entrypoints #1479

wants to merge 1 commit into from

Conversation

javierggt
Copy link
Contributor

@javierggt javierggt commented Jan 22, 2025

This PR sets some entrypoints explicitly. This branch was already used to build the corresponding packages, and they are already in the test conda channel. They were used to test ska3-matlab 2025.2rc1 on windows.

To merge this, I would actually remove the commit where I set the build number.

@jeanconn
Copy link
Contributor

I've found it is hard to know what to do with build numbers - they are handy in testing to make sure the right package is being used for testing. But if that package with the incremented build number is the one that is tested, should we just stick with the incremented build and then decrement it sometime before the next package release? Unclear.

@javierggt
Copy link
Contributor Author

That is why I said I would remove that commit, but it helps to have it in the branch, because then one can build using the workflows. I do not know of a better way either.

@taldcroft
Copy link
Member

This looks fine, but I do find the two sources of entry points (setup.py and here) confusing (which one wins?), and of course ripe for a mismatch.

Just out of curiosity, would using pyproject.toml alleviate the need for entry points in the build recipe? I.e. do things work better with that?

@jzuhone
Copy link
Collaborator

jzuhone commented Feb 20, 2025

Yeah, I don't understand why the entry points need to be both here and in the package.

@jeanconn
Copy link
Contributor

For @jzuhone, for history, this came up due to the fact that the with the current versions of the tools (setuptools and conda), conda does not know to make arch-appropriate entry points unless they are called out in the recipe (and if it doesn't do this, know about them, setuptools just installs broken scripts on windows), and if the entrypoints aren't in the setup.py or pyproject.toml, it is difficult to do local pip installs.

To your question @taldcroft , according to our slack conversation on this @javierggt tried pyproject.toml and it didn't fix the scripts on windows. I thought testing confirmed it was benign to have the entry points called out in both places since this is how the packages for the matlab release were made and it didn't break anything, but I suppose we could explicitly test crossmatched naming going forward to see which one "wins" as you say.

One bonus of calling out the entry-points in the conda recipe is that you can explicitly convert a pure-python package back to conda noarch for purposes of distribution. However it seems like we could also leave these as arch packages and perhaps call out the entry points as windows-only (the platform with the issue driving this change). I don't know if reducing the scope would actually make anything simpler.

@taldcroft
Copy link
Member

Thanks @jeanconn. As a sanity check, I took a look at what astropy does (for pip and conda), and the answer is that they have exactly the same repetition:
https://github.com/search?q=org%3Aastropy%20astropy.io.fits.scripts.fitsdiff&type=code

So there does not appear to be a magic recipe for single-sourcing entry point scripts, so I'm satisfied that the current approach is the best we can do for now.

@javierggt
Copy link
Contributor Author

@taldcroft you asked a good question: which one wins?

I have not checked what happens in case of discrepancies. Discrepancies can be:

  • an entry point missing in one of the sources.
  • the same entrypoint referring to different functions in the two sources.

I think it would be good to know for sure before merging.

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