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

Unixify paths for more cases #70

Merged
merged 4 commits into from
Jun 16, 2023
Merged

Conversation

huangqinjin
Copy link
Contributor

  • Unixify paths for cl /showIncludes only. Otherwise outputs may be partially converted, e.g.

    > echo '__FILE__;' > /tmp/test.c && /opt/msvc/bin/x64/cl /nologo /E /FC /tmp/test.c
    test.c
    #line 1 "\\tmp\\test.c"
    "\\tmp\\test.c";
  • Unixify paths for dumpbin /PDBPATH also. Otherwise outputs were also partially converted, e.g.

    > echo 'int main() {}' > /tmp/main.c && /opt/msvc/bin/x64/cl /nologo /Z7 /tmp/main.c /Fe/tmp/main.exe && /opt/msvc/bin/x64/dumpbin /nologo /PDBPATH /tmp/main.exe
    main.c
    
    Dump of file \tmp\main.exe
    
    File Type: EXECUTABLE IMAGE
      PDB file found at '\tmp\main.pdb'
    

    Vcpkg use it to implement the script vcpkg_copy_pdbs: https://github.com/microsoft/vcpkg/blob/662dbb50e63af15baa2909b7eac5b1b87e86a0aa/scripts/cmake/vcpkg_copy_pdbs.cmake#L24

  • Unixify paths for cl /P /Fi. There are three cases of cl /P:
    i: A output file is specified by /Fi. This PR handles this case and resolves Work with ccache: Problem with MSVC /P (Preprocess to a File) option #69 .
    ii. & ii: One or more source files are given to cl /P and an optional output directory can be specified by /Fi (default to CWD). cl will generate {output directory}/{basename of a source file without extension}.i for each source file. This PR does NOT address these cases.

@huangqinjin
Copy link
Contributor Author

@mstorsjo It is worth unit testing these changes. Could you please add a basic test for /showIncludes? And then I will complete the tests.

@mstorsjo
Copy link
Owner

mstorsjo commented Jun 4, 2023

(Sorry I don't have time to actually review this one right now, I'll have to get back to it later.)

It is worth unit testing these changes. Could you please add a basic test for /showIncludes? And then I will complete the tests.

Hmm, what would be a good kind of test here? The practical tests would be of the form to just build something with e.g. ninja, and then rebuilding and making sure that it works (doesn't error out due to references to unknown paths), doesn't rebuild things needlessly, and actually does rebuild files as necessary if e.g. touching a header. But many of these aspects are hard and messy to verify in a test...

I guess what you have in mind is to manually run cl /showIncludes and inspect the output from it, and verify some properties of it? I'll see what I can do wrt that - I can hopefully give that a try within a few days.

@huangqinjin
Copy link
Contributor Author

manually run cl /showIncludes and inspect the output from it

Yes, this is what in my mind. For example, run

$ touch /tmp/header.h && echo '#include "header.h"' >/tmp/test.c && /opt/msvc/bin/x64/cl /nologo /showIncludes /c /tmp/test.c
test.c
Note: including file: /tmp/header.h

Save the output to a file as reference. The test just runs the same command, captures the outputs, compares to references byte-wise.

@mstorsjo
Copy link
Owner

mstorsjo commented Jun 8, 2023

manually run cl /showIncludes and inspect the output from it

Yes, this is what in my mind. For example, run

$ touch /tmp/header.h && echo '#include "header.h"' >/tmp/test.c && /opt/msvc/bin/x64/cl /nologo /showIncludes /c /tmp/test.c
test.c
Note: including file: /tmp/header.h

Save the output to a file as reference. The test just runs the same command, captures the outputs, compares to references byte-wise.

Ok, does db57d18 seem reasonable?

@mstorsjo
Copy link
Owner

mstorsjo commented Jun 9, 2023

Ok, does db57d18 seem reasonable?

I realized we definitely should run the same kind of test on macOS too. There are too many subtle differences in how sed behaves between GNU and (old) BSD tools...

wrappers/wine-msvc.sh Outdated Show resolved Hide resolved
wrappers/wine-msvc.sh Outdated Show resolved Hide resolved
@huangqinjin
Copy link
Contributor Author

run the same kind of test on macOS too

Yes. I think we should write some source files, some reference files, and some scripts to test it.

@mstorsjo
Copy link
Owner

mstorsjo commented Jun 9, 2023

run the same kind of test on macOS too

Yes. I think we should write some source files, some reference files, and some scripts to test it.

Yes, that sounds reasonable - feel free to try to build something such!

@huangqinjin
Copy link
Contributor Author

I am thinking that, we can move the postprocesses to their corresponding wrappers. And export two environment variables WINE_MSVC_STDOUT_SED and WINE_MSVC_STDERR_SED from each wrapper and use them in the sed commands in wine-msvc.sh.

@mstorsjo
Copy link
Owner

I am thinking that, we can move the postprocesses to their corresponding wrappers. And export two environment variables WINE_MSVC_STDOUT_SED and WINE_MSVC_STDERR_SED from each wrapper and use them in the sed commands in wine-msvc.sh.

I guess that sounds reasonable.

@huangqinjin huangqinjin force-pushed the unixify-paths branch 3 times, most recently from 056c551 to 2491b10 Compare June 15, 2023 10:11
@huangqinjin
Copy link
Contributor Author

huangqinjin commented Jun 15, 2023

I added tests for each commit and switched to ERE (Extended Regular Expression).

Quote from https://www.gnu.org/software/sed/manual/html_node/BRE-vs-ERE.html#BRE-vs-ERE:

In GNU sed, the only difference between basic and extended regular expressions is in the behavior of a few special characters: ‘?’, ‘+’, parentheses, braces (‘{}’), and ‘|’.

The reason I made the switch is that I want to use | (alternation) to simplify sed expressions. GNU sed supports \| in BRE but BSD sed does not.

There are too many subtle differences in how sed behaves between GNU and (old) BSD tools...

Definitely right. There is a great answer https://stackoverflow.com/a/24276470 for reference. Things I learnt and applied:

  • in-place updating:
    • BSD sed: -i ''
    • GNU sed: -i'' or -i
  • escape sequence \t: BSD sed does not support. Use [:blank:] instead.
  • function list {p; g;}: BSD sed requires ; after last function but GNU sed does not.

@huangqinjin huangqinjin marked this pull request as ready for review June 15, 2023 15:46
Copy link
Owner

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good to me now, thanks! Just a couple details for discussion.

wrappers/wine-msvc.sh Show resolved Hide resolved
wrappers/cl Outdated Show resolved Hide resolved
wrappers/cl Show resolved Hide resolved
@mstorsjo
Copy link
Owner

LGTM, thanks!

@mstorsjo mstorsjo merged commit 7bdfce8 into mstorsjo:master Jun 16, 2023
4 checks passed
@huangqinjin huangqinjin deleted the unixify-paths branch June 16, 2023 13:40
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.

Work with ccache: Problem with MSVC /P (Preprocess to a File) option
2 participants