Add support for packaging binaries in scripts data directory #248
Conversation
setuptools_rust/extension.py
Outdated
| base_mod, name = mod.rsplit(".") | ||
| script = "%s=%s.%s:run" % (name, base_mod, _script_name(executable)) | ||
| entry_points.append(script) | ||
| if "." in mod: |
There was a problem hiding this comment.
I don't like this, I'm thinking about change script=False to this the behavior of this PR.
IMO the current Binding.Exec with script=False build mode doesn't not much make sense since it only build the binary and put in somewhere in the package.
There was a problem hiding this comment.
Trying to get my head around this comment. I agree that script=False is kind of weird / useless at the moment. Is there genuinely a use case where the current behaviour of placing the binary into site-packages is desirable?
On the other hand, if we change script=False to match this PR - I understand that this would mean that the binary would by default be copied into the bin directory (and added to PATH, essentially). Once we have that, is there ever a reason to want to use script=True?
There was a problem hiding this comment.
IMO most users reach to Exec binding and script=True to put the binary in PATH, otherwise it's not very useful.
In maturin we also build bin bindings by putting it in script data directory directly.
Here is the original issue for the current implementation: #22
There was a problem hiding this comment.
So if we change script=False to work like maturin is there a reason to use script=True ever? Maybe we can simplify and deprecate the argument?
There was a problem hiding this comment.
So if we change script=False to work like maturin is there a reason to use script=True ever?
I don't think there is any.
Maybe we can simplify and deprecate the argument?
I think so.
According to the docs, target accepts Union[str, Dict[str, str]] right now
target
(Union[str, Dict[str, str]])– The full Python dotted name of the extension, including any packages, i.e not a filename or pathname. It is possible to specify multiple binaries, if extension uses Binding.Exec binding mode. In that case first argument has to be dictionary. Keys of the dictionary correspond to the rust binary names and values are the full dotted name to place the executable inside the python package. To install executables with kebab-case names, the final part of the dotted name can be in kebab-case. For example, hello_world.hello-world will install an executable named hello-world.
I'd also like to change it to accept List[str] and deprecate Dict[str, str] for multiple binary names.
There was a problem hiding this comment.
I wonder what if we went further and made a RustBin class to use like RustExtension? Then no need to accept a List at all I think, users can write the list at the setup() level.
There was a problem hiding this comment.
A RustBin class is nice, I'll give it a try.
There was a problem hiding this comment.
(I'm thinking that way maybe we can leave the existing behavior and emit warnings for deprecation, and then the RustBin class can have the new behavior which we want to have going forward. Gives users time to tell us they need the old way before we break / remove.)
|
Hopefully this will resolve conda-forge/maturin-feedstock#60 (comment) |
|
@messense, sorry for the slow response from me. See thread above - I think there's definitely room to improve the situation here, let's try and figure it out what we want to get and hopefully we can unblock the maturin conda feedstock swiftly. I'll try to reply quicker on this thread now! |
|
Hmmm, |
cross released 0.2.2 recently, something might be broken, I'm going to pin it to 0.2.1 for now and investigate later. |
Opened cross-rs/cross#864 |
Closes #247