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

Fix MSYS=wincmdln, and enable it by default #10

Merged
merged 2 commits into from
Aug 14, 2020

Conversation

dscho
Copy link
Collaborator

@dscho dscho commented Aug 11, 2020

This is apparently broken since 2015...

…nvironment variables to Windows form for native Win32 applications.

This reinstates the MSYS=wincmdln functionality that was inadvertently
broken by the patch: with that setting, all processes spawned by MSYS2
are supposed to fill the CommandLine field in the process information so
that e.g. `wmic process list` can see it (otherwise, only the executable
itself is listed in the command line, but all the arguments are
missing).

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho requested review from lazka, elieux and Alexpux August 11, 2020 19:27
@dscho
Copy link
Collaborator Author

dscho commented Aug 11, 2020

I marked this up as a fixup! so that the next rebase (provided that git rebase -i is used, with auto-squash in effect) squashes these changes into the appropriate patch.

FWIW I wonder whether we should also make wincmdln the default for MSYS2?

@lazka
Copy link
Member

lazka commented Aug 11, 2020

If I understand this correctly this is for non-cygwin binaries in which case the path would have been hit anyway because !real_path.iscygexec(). Am I missing something?

(unrelated, as you might have noticed I've changed the default branch for this repo and added CI. I hope that's ok)

@elieux
Copy link
Member

elieux commented Aug 11, 2020

Based on my debugging experience, missing command line arguments are an issue in cygwin processes, not Win32 ones.

@elieux
Copy link
Member

elieux commented Aug 11, 2020

Assuming there's no disadvantage to this feature, I would like this turned on by default (or hard-coded even).

@dscho
Copy link
Collaborator Author

dscho commented Aug 12, 2020

If I understand this correctly this is for non-cygwin binaries in which case the path would have been hit anyway because !real_path.iscygexec(). Am I missing something?

No, this is for MSYS2/Cygwin binaries. If you call, say, vim.exe abc from a regular MSYS2 Bash, you won't see the command-line arguments in the output of wmic process where "CommandLine like '%vim%'" get CommandLine (see git-for-windows/git#2756 for more details).

Assuming there's no disadvantage to this feature, I would like this turned on by default (or hard-coded even).

There is only one disadvantage that I can see: as the shown command-line is not the actual command-line as used by MSYS2/Cygwin, there might be discrepancies. The most obvious one comes when the command-line is longer than 32k: this is allowed in MSYS2 when calling another MSYS2 process, but it would be shown truncated by wmic.

However, I think you're right: this should be the default nevertheless because the surprise of not seeing any command-line arguments is bigger than the surprise of seeing a potentially truncated one.

Having said that, I do want to give users an "escape hatch" in case they absolutely want Cygwin's default behavior, so I am more in favor of just switching the default and support opting out (via MSYS=nowincmdln). Sound okay?

@dscho dscho changed the title Fix MSYS=wincmdln Fix MSYS=wincmdln, and enable it by default Aug 12, 2020
@dscho
Copy link
Collaborator Author

dscho commented Aug 12, 2020

I just added a commit to enable MSYS=wincmdln by default, and verified that it works.

@elieux
Copy link
Member

elieux commented Aug 12, 2020

Having said that, I do want to give users an "escape hatch" in case they absolutely want Cygwin's default behavior, so I am more in favor of just switching the default and support opting out (via MSYS=nowincmdln). Sound okay?

Okey dokey.

@dscho
Copy link
Collaborator Author

dscho commented Aug 12, 2020

@elieux @lazka so... any chance one of you could review these two patches (and approve)?

@elieux
Copy link
Member

elieux commented Aug 12, 2020

Sure. But did you add that second commit? I can't see it.

@lazka
Copy link
Member

lazka commented Aug 12, 2020

No, this is for MSYS2/Cygwin binaries. If you call, say, vim.exe abc from a regular MSYS2 Bash, you won't see the command-line arguments in the output of wmic process where "CommandLine like '%vim%'" get CommandLine (see git-for-windows/git#2756 for more details).

ok, I was confused since the commit says "for native Win32 applications". Thanks for the explanation. With my very limited cpp/cygwin experience, your explanation makes sense, so lgtm.

@dscho
Copy link
Collaborator Author

dscho commented Aug 13, 2020

did you add that second commit? I can't see it.

I had pushed it, but to another remote. D'oh! Sorry ;-)

@dscho dscho force-pushed the fix-wincmdln-in-msys2 branch 2 times, most recently from 1a17369 to fae9acd Compare August 13, 2020 09:49
@dscho
Copy link
Collaborator Author

dscho commented Aug 13, 2020

FWIW this is the second commit I was talking about.

@elieux
Copy link
Member

elieux commented Aug 13, 2020

Should the docs be updated or do we not care about them?

@elieux
Copy link
Member

elieux commented Aug 13, 2020

<listitem>
<para><envar>(no)wincmdln</envar> - if set, the windows complete command
line (truncated to ~32K) will be passed on any processes that it creates
in addition to the normal UNIX argv list.  Defaults to not set.</para>
</listitem>

@elieux
Copy link
Member

elieux commented Aug 13, 2020

It is hard for me to follow the code, but comparing our current code and your patched code both to upstream code, yours seems to be more logical. I'm assuming it builds and does what it says on the tin.

In the Cygwin project, it was decided that the command-line of Cygwin
processes, as shown in the output of `wmic process list`, would suffer
from being truncated to 32k (and is transmitted to the child process via
a different mechanism, anyway), and therefore only the absolute path of
the executable is shown by default.

Users who would like to see the full command-line (even if it is
truncated) are expected to set `CYGWIN=wincmdln` (or, in MSYS2's case,
`MSYS=wincmdln`).

Seeing as MSYS2 tries to integrate much better with the surrounding
Win32 ecosystem than Cygwin, it makes sense to turn this on by default.

Users who wish to suppress it can still set `MSYS=nowincmdln`.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho force-pushed the fix-wincmdln-in-msys2 branch from fae9acd to d1fe488 Compare August 14, 2020 07:27
@dscho
Copy link
Collaborator Author

dscho commented Aug 14, 2020

Should the docs be updated or do we not care about them?

I was on the fence, as the documentation still talks about CYGWIN while we ignore that environment variable and look at MSYS instead.

It is hard for me to follow the code, but comparing our current code and your patched code both to upstream code, yours seems to be more logical.

Basically, @Alexpux' patch erroneously moves the if (wincmdln ...) block from being a sibling of the if (real_path.iscygexec ()) block to an else arm of the latter. For details, see 91990da#diff-b72d3b2289a1e6410a2602d0fa250a3cL386-L395 (and expand the preceding context a bit).

My patch moves the if (wincmdln ...) block out of that else arm again, so that it also applies to spawned MSYS2 processes.

I'm assuming it builds and does what it says on the tin.

Yes, I tested (and just re-tested, to be sure) the built msys0.dll, verifying that it does what I claim in this PR.

@dscho dscho merged commit 5f10a64 into msys2:msys2-3_1_6-release Aug 14, 2020
@dscho dscho deleted the fix-wincmdln-in-msys2 branch August 14, 2020 12:48
dscho added a commit to dscho/MSYS2-packages that referenced this pull request Aug 14, 2020
This ports the patches from msys2/msys2-runtime#10 to
the actual package definition used to build the `msys2-runtime` package.

Signed-off-by: Johannes Schindelin <[email protected]>
@lazka
Copy link
Member

lazka commented Aug 14, 2020

offtopic, would it make sense to rename this repo to msys2-runtime? To make the relation to the package more clear?

@dscho
Copy link
Collaborator Author

dscho commented Aug 17, 2020

offtopic, would it make sense to rename this repo to msys2-runtime? To make the relation to the package more clear?

Sure. In Git for Windows, we did exactly that: https://github.com/git-for-windows/msys2-runtime/

@lazka
Copy link
Member

lazka commented Aug 17, 2020

👍 done

github-actions bot pushed a commit that referenced this pull request Dec 14, 2023
gcc-14 will default to c99 and as a result a fair amount of old code in newlib
(particularly libgloss) is failing to build.  I don't offhand know how many
patches will be necessary to fix the various failures. I'll just pick them off
one by one from my tree.

This particular patch works around the return-mismatch problem syscalls.c for
fr30.

That file is a bit odd in that most functions are declared as returning an
integer, but the implementations look like:

> int
> _read (file, ptr, len)
>      int    file;
>      char * ptr;
>      int    len;
> {
>   asm ("ldi:8 %0, r0" :: "i" (SYS_read) : "r0");
>   asm ("int   #10");
>
>   return;
> }

Note the lack of a value on the "return" statement.  The assumption is that the
interrupt handler implementing syscalls will put the return value into the
proper register, so falling off the end of the C function or returning with no
value works in the expected way.  It's not good code, but it probably works.

Working from that assumption I decided to just use a pragma to disable the
upgraded diagnostic from GCC -- essentially preserving existing behavior.

This is the only fr30 specific issue that needs to be resolved and the only
issue (so far) I've seen of this specific nature.
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.

3 participants