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

Avoid wildcard expansion when making F# builds #3421

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

jonahbeckford
Copy link

This fixes a regression introduced in af1b98d. It was discovered testing Windows CI in #3402 and is a blocker for that PR.

Before the regression there was the term:

--already_cached '*,'

The regression switched it to:

--already_cached '*'

which causes:

Y:/source/FStar/src/ocaml-output/fstar/bin/fstar.exe   --use_hints   --warn_error @241 --cache_checked_modules --odir fs/extracted --cache_dir .cache --already_cached '*' FStar.Pervasives.fst --codegen FSharp --extract_module FStar.Pervasives
* Error 151:
  - Not a valid FStar file: '..'

1 error was reported (see above)
make: *** [Makefile.extract.fsharp:34: fs/extracted/FStar_Pervasives.fs] Error 1

This fix puts back the original trailing comma which inhibits the wildcard expansion.

This fixes a regression introduced in
FStarLang@af1b98d

Before the regression there was the term:
  --already_cached '*,'

The regression switched it to:
  --already_cached '*'

which causes:

  Y:/source/FStar/src/ocaml-output/fstar/bin/fstar.exe   --use_hints   --warn_error @241 --cache_checked_modules --odir fs/extracted --cache_dir .cache --already_cached '*' FStar.Pervasives.fst --codegen FSharp --extract_module FStar.Pervasives
  * Error 151:
    - Not a valid FStar file: '..'

  1 error was reported (see above)
  make: *** [Makefile.extract.fsharp:34: fs/extracted/FStar_Pervasives.fs] Error 1

This fix puts back the original trailing comma which inhibits the
wildcard expansion.
@mtzguido
Copy link
Member

Hi Jonah, thanks for this! And sorry I broke it...

How is this being expanded? Is it running on powershell maybe? Bash and other Unix shells wouldn't expand under the single quotes.

Patch is good in any case, just curious. Thanks!

@jonahbeckford
Copy link
Author

Yes, make -f Makefile.extract.fsharp dll is being run in PowerShell, but I think the Makefile default SHELL is the Command Prompt interpreter on Windows per https://www.gnu.org/software/make/manual/html_node/Choosing-the-Shell.html.

So it could be PowerShell or Command Prompt that is seeing the * but I think it is Command Prompt.

However, Command Prompt (DOS) itself does not expand wildcards. Unlike Unix, the entire command line argument string is given to the executable (ex. fstar.exe) and the executable is responsible for expanding wildcards. I forget the Win32 arcane library calls that can do the expansion, but the OCaml linker is probably getting expansion done inside fstar.exe. Confer: ocaml/ocaml#7473

@tahina-pro
Copy link
Member

tahina-pro commented Aug 28, 2024

From what Jonathan Protzenko told me back then, Windows expands wildcards even with cmd, see for instance FStarLang/karamel@b6aed59, which was a fix to the Karamel tool to allow a special syntax for \* instead of * in its arguments

@jonahbeckford
Copy link
Author

Small world. I think the @protz in the OCaml bug report ocaml/ocaml#7473 is the same you mentioned for the karamel issue.

@tahina-pro
Copy link
Member

Small world. I think the @protz in the OCaml bug report ocaml/ocaml#7473 is the same you mentioned for the karamel issue.

Yes, he uses @msprotz for work-related tasks, and @protz elsewhere.

@msprotz
Copy link
Collaborator

msprotz commented Aug 28, 2024

Glad that I'll be remembered on the internet for digging into the shell expansion semantics of Cygwin vs native programs. See also https://stackoverflow.com/questions/9946586/cygwin-bash-c-yields-different-argc-depending-on-whether-the-command-start-with for another entry that I hope will make it into my collected works.

@mtzguido mtzguido merged commit f062904 into FStarLang:master Aug 28, 2024
2 checks passed
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