-
Notifications
You must be signed in to change notification settings - Fork 722
cabal-install: do not pass the executable name to cabal external commands #11232
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
base: master
Are you sure you want to change the base?
Conversation
47ea2c6
to
50e1bfa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an issue here which hasn't been brought up yet, which is one of the reasons the program name is passed as the first parameter.
See the caveat on getProgName
. On Windows in particular, there is no way to retrieve argv[0]
, so anything relying on it will have issues.
This also plays into a potential future change where invocation via cabal
would be marked. (Likely by something like how login
on Unix marks login shells: leading '-'
.) The reason we would want this is the potential for future versions of the external command mechanism to receive bits of project or cabal state via the environment. (We already pass $CABAL
/ %CABAL%
, but tooling should already use that to ensure they are running the right cabal
.) External commands would need some "key" indicating that the additional information is available. We don't do this yet because we're waiting for feedback from users of the external command mechanism about what information they would need from cabal
.
All of which means I'm not sure we can get away with not passing the name. At minimum, we might need to replace it with a --run-from-cabal
parameter or other mechanism to indicate that additional information is available, and potentially parameterized by the actual run name to work around getProgName
issues on non-POSIX (which would put us right back where we are now).
There are two possible workarounds listed, the other one (just using multiple executables) even aligns very nicely with how Haskell projects are usually structured and doesn't incur a lot of overhead. Does that one not work on windows either? On the topic of getting more information: if you're setting an environment variable anyway, then why is it not possible to just check for the presence of that variable? |
If two tools share most of their implementation, multiple driver programs are possible but may lead to complex library entry points. This is why Unix likes to use (sym)links in that case, but that doesn't fly on Windows and probably other OSes that aren't Unix-like (there's ongoing work to port ghc and build tools to Haiku, for example). |
You haven't updated the external command tests (PackageTests/ExternalCommandExitCode/cabal.test.hs, PackageTests/ExternalCommand/cabal.test.hs, PackageTests/ExternalCommandHelp/cabal.test.hs). |
I didn't see it failing locally, must've slipped through the one million lines of output
That's why I said in Haskell, it is super common to make the executable just a shim that invokes the library. In my opinion we don't have too look too closely what other people are doing there because they have other backgrounds. The default case is that it's very simple to add new executables to Haskell projects and even if that's not a case, for that hypothetical project (I know of none that has this problem) it's not a hurdle that big that should force other users to take the hurdles that they have to take now. (Or making anything impossible)
Yes, I propose not setting that variable and instead use a longer, more rarely used one like $CABAL_EXTERNAL_COMMAND_CONTEXT which at the same time would be good to avoid executables that currently long for $CABAL to break. |
We still need to set it, because if a user runs a different |
I doubt it's ever wise to use |
That may tie your program to a specific version. In the opposite direction, this is why I think we'll be exporting more information in the future. Also, future versions of cabal will have more stable interfaces for this, whose design will come from this and other users (e.g. HLS); we already have a ticket and some early design work for a proposed |
Do I understand this incorrectly though: I think this is orthogonal to the issue at hand - the additional argument wouldn't carry any information about the status of cabal, it would be the environment variable. |
If the additional parameter were to stay, it would be the same as the |
Some care is needed with the environment, though; it's already pretty sizable on some systems, and there is a limit on |
Which is something under mine control as a developer, so it's good. While with |
I feel like you're severily misunderstanding the docs here. Here's what it looks like:
|
50e1bfa
to
2fbabfe
Compare
It still leaves out cabal-specific changes to it to indicate that it has been run from cabal and extra information may be available (#11232 (review)); it only provides the executable name that was run. |
Yes. This is a completely orthogonal issue though to whether cabal copies the command name in argv[1] or not. This PR isn't about making the perfect cabal external commands system, it's about changing that one specific aspect. |
Previously the executable name of the external command was passed to external commands as the first argument. This behaviour was adapated from cargo which does this because of reasons that are internal to rust that do not affect GHC Haskell, and are even orthogonal to patterns that see common use in Haskell. Additionally, it complicates the 'simple' case which is what we should optimize for when building such a feature. The previous use case (one executable that serves multiple external subcommands) is still possible by the following means: - using a wrapper around the executable - using a symlink and check argv[0] in the executable Resolves haskell#10275
2fbabfe
to
cdd2f7a
Compare
Thank you for submitting the patch @MangoIV! I agree with your arguments and I wasn't able to discern serious counter-arguments from the discussion above. So I'm in support of the change. Why is it a draft? |
There's two missing checkboxes that I haven't gone through yet (I want to adjust missing docs) |
You still also have a typo in your changelog entry and two tests that still need to be fixed (I saw only one fixed in the last push). |
yeah that's also why it's still draft, sorry. I will of course go through this. :) |
No worries, I was just making sure it's not an oversight. |
Previously the executable name of the external command was passed to external commands as the first argument.
This behaviour was adapated from cargo which does this because of reasons that are internal to rust that do not affect GHC Haskell, and are even orthogonal to patterns that see common use in Haskell.
Additionally, it complicates the 'simple' case which is what we should optimize for when building such a feature.
The previous use case (one executable that serves multiple external subcommands) is still possible by the following means:
Include the following checklist in your PR:
significance: significant
in the changelog file.