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

Mention the proper env vars for usage in MinGW #404

Merged
merged 1 commit into from
Jan 2, 2024
Merged

Mention the proper env vars for usage in MinGW #404

merged 1 commit into from
Jan 2, 2024

Conversation

jasagredo
Copy link
Contributor

@jasagredo jasagredo commented Dec 14, 2023

If one doesn't set that flag, by default any argument /.../ gets converted to a path by MSYS2 (see).

❯ cabal run my-testsuite -- -p "/my pattern/"
option -p: Could not parse: C:/msys64/my pattern/ is not a valid pattern
...
❯ MSYS2_ARG_CONV_EXCL=* cabal run my-testsuite -- -p "/my pattern/"
<it works>

This will print the following advise on failing tests when running on what looks like a MinGW enviroment:

          Use -p '/my pattern/' to rerun this test only.
          If running in a MinGW shell, set the environment variable 'MSYS2_ARG_CONV_EXCL=*' or the pattern above will be mangled.

It still uses the "If" formulation because PowerShell running the mingw haskell toolchain won't need this parameter (modulo haskell/cabal#9519):

PS C:\Users\Javier\example> cabal run my-testsuite -- -p "/my pattern/"
<works>

@Bodigrim
Copy link
Collaborator

FAQ recommends setting MSYS_NO_PATHCONV=1. Is MSYS2_ARG_CONV_EXCL=* better or different?

It would be better to add the instruction to the error message for failed parse instead of repeating it after each failed test.

@jasagredo
Copy link
Contributor Author

I was unable to make it work with MSYS_NO_PATHCONV but I can try again.

And I agree it would be better to do it in the parse error message. Good point, will do.

@jasagredo
Copy link
Contributor Author

❯ MSYS_NO_PATHCONV=1 cabal run cardano-test -- -p "/ExtLedgerState_Conway/"
option -p: Could not parse: C:/msys64/ExtLedgerState_Conway/ is not a valid pattern
...

❯ MSYS2_NO_PATHCONV=1 cabal run cardano-test -- -p "/ExtLedgerState_Conway/"
option -p: Could not parse: C:/msys64/ExtLedgerState_Conway/ is not a valid pattern
...

@jasagredo
Copy link
Contributor Author

As the error in parsing is created by mkOptionCLParser using just the name of the parser, in order to not print this error for all parsers (which wouldn't make sense), would it be better to add a field to IsOption v something like:

class Typeable v => IsOption v where
  -- | Optional additional hints to present on the CLI when this option 
  -- fails to parse, for example OS specific hints.
  additionalHints :: Tagged v (Maybe String)

?

@Bodigrim
Copy link
Collaborator

Maybe we can unmangle automatically what MSYS has mangled? That would be the best solution.

@jasagredo
Copy link
Contributor Author

I don't think that would be a good approach, I think tasty should try to interpret the input as a pattern always, and fail if it is not a pattern.

At most the slashes could be optional maybe? That would avoid MSYS2 mangling the parameter.

(The above is my current opinion, I don't know the internals of tasty or the overall design and goals so I could be wrong)

@Bodigrim
Copy link
Collaborator

Bodigrim commented Dec 17, 2023

MSYS_NO_PATHCONV used to work for me in MinGW shells like Git Bash. It’s surprising that it does not work for you, has something changed on MSYS side?..

There are two low-hanging fruits: we can update README.md with more robust instructions and we can also extend --help output wrt --pattern. Other than that I’d prefer to extend the parser of patterns to demangle things: after all a user might not be in position to manipulate environment variables at all.

@jasagredo
Copy link
Contributor Author

I found why the env var does not work. It is specific to the MSYS2 fork of Git for Windows git-for-windows/msys2-runtime@f874ac108a and doesn't apply when one installs MinGW directly instead of via Git for Windows.

@jasagredo
Copy link
Contributor Author

Let's just update the README and let it be.

@jasagredo
Copy link
Contributor Author

Let me know if this looks good now.

@Bodigrim Bodigrim merged commit c7396fa into UnkindPartition:master Jan 2, 2024
13 checks passed
@Bodigrim
Copy link
Collaborator

Bodigrim commented Jan 2, 2024

Thanks!

@jasagredo jasagredo deleted the js/msys2-conv branch February 19, 2024 12:45
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.

2 participants