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

external commands need to explicitly handle their own name being passed to the executable #10275

Open
MangoIV opened this issue Aug 25, 2024 · 10 comments

Comments

@MangoIV
Copy link
Contributor

MangoIV commented Aug 25, 2024

Describe the bug

#9412 passes name to the external executable. The use case is a symlink to an executable that handles multiple commands.

I think this use-case doesn't justify the complication this causes for the average case and it isn't strictly necessary to support the use-case.

To Reproduce

  1. have a command cabal-bla in your environment that doesn't allow to take the argument "bla"
  2. run cabal bla
  3. should fail with "doesn't expect "bla""

Expected behavior

cabal bla should be equivalent to cabal-bla

To support the intended use case

If you want to have a singular exe handle multiple plugin arguments, just write a wrapper that invokes the particular exe with the argument and symlink the bash wrapper. This behavior should not go at the cost of the average case behavior.

Additional context
The fix should be easy, just don't pass name to the executable here:

result <- try $ createProcess ((proc exec (name : cmdArgs)){env = Just new_env})

(@mpickering)

@ulysses4ever
Copy link
Collaborator

The context is in #9405 Apparently, this was modeled after cargo. I don't have a strong preference either way.

@Bodigrim
Copy link
Collaborator

I was somewhat surprised by this behavior too, but it's not difficult to workaround, see https://github.com/Bodigrim/cabal-add/blob/267eb6802477076b64568bff774aa7833bcbfa7b/app/Main.hs#L186-L192.

@MangoIV
Copy link
Contributor Author

MangoIV commented Aug 25, 2024

I don’t think the average case should work around the special case if it isn’t absolutely necessary which I don’t see, tbh.

@MangoIV
Copy link
Contributor Author

MangoIV commented Aug 25, 2024

Maybe I can understand better when I read why cargo did it like this…

@mpickering
Copy link
Collaborator

It seemed sensible to me to implement the external command system exactly like the cargo system. There are less surprises if you are already familiar with that and it is likely they have worked out any usability issues.

@MangoIV
Copy link
Contributor Author

MangoIV commented Aug 27, 2024

As much as I like the rust tooling, I honestly don't understand this and I don't necessarily think it's a good idea to copy something without a clear reason why.

Do you agree with the general notion that the average case should not be complicated at the benefit of some exotic case that can somewhat trivially be worked around?

@mpickering
Copy link
Collaborator

@MangoIV I am not particularly invested in the design, it seems a very minor task to drop the first argument (and something which is easily discoverable by reading the short documentation section).

It does seem more complicated to me to provide a wrapper which works across platforms and calls an executable forwarding on the right commands.

Here is a similar issue on cargo which was just closed (rust-lang/cargo#9373).

If someone wishes to change the design then I'm not going to object but it does seem like creating unecessary work to me. In my opinion, there is a large benefit to uniformity across ecosystems when it is possible to do so. Making Haskell work as much like the next tool when it comes to packaging and distribution is more important that forging an independent path.

@fgaz
Copy link
Member

fgaz commented Aug 27, 2024

If you want to have a singular exe handle multiple plugin arguments, just write a wrapper

You could even just use argv[0], like coreutils does.


I, too, would prefer the name not to be passed as argv[1]. It interferes with direct use of external commands. As a counterexample to cargo, git behaves like @MangoIV's proposal.

However, the feature is already out. Changing it now would unnecessarily break existing tools.

@MangoIV
Copy link
Contributor Author

MangoIV commented Aug 27, 2024

I was somewhat surprised by this behavior too, but it's not difficult to workaround, see https://github.com/Bodigrim/cabal-add/blob/267eb6802477076b64568bff774aa7833bcbfa7b/app/Main.hs#L186-L192.

@Bodigrim what would the workaround be if the package you want to add is called add and cabal-add is called as a standalone executable? That would certainly not work right now and you would have to read an env-var?

@Bodigrim
Copy link
Collaborator

@MangoIV good catch! Thanks, fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants