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

Absolute paths in response files also need to be handled #93

Open
marcoesposito1988 opened this issue Oct 26, 2023 · 4 comments
Open

Absolute paths in response files also need to be handled #93

marcoesposito1988 opened this issue Oct 26, 2023 · 4 comments

Comments

@marcoesposito1988
Copy link

The relatively low limit on command line length of the Windows tools can create problems when linking large libraries. link.exe was silently hanging in my case, and I spent hours pulling out my hair to understand why.

The typical way of dealing with this is using response files: the content of the command line is written to a file, and the path to this file is passed instead of the arguments as link.exe @Path/To/Response/File.rsp .... However, when cross-compiling with wine this doesn't work out of the box because Ninja thinks that we are on Linux and hence we don't need to do this by default.

A workaround when using Ninja is setting CMAKE_NINJA_FORCE_RESPONSE_FILE, which will do the correct thing. We then have another problem, which is that the absolute paths in the response file are not in "wine format" (beginning with Z:), because they are not intercepted by the wrapper script.

I think I found a solution, and I have been using it without problems so far. Shall I open a PR, or do you see a problem with this approach?

@mstorsjo
Copy link
Owner

The relatively low limit on command line length of the Windows tools can create problems when linking large libraries. link.exe was silently hanging in my case, and I spent hours pulling out my hair to understand why.

The typical way of dealing with this is using response files: the content of the command line is written to a file, and the path to this file is passed instead of the arguments as link.exe @Path/To/Response/File.rsp .... However, when cross-compiling with wine this doesn't work out of the box because Ninja thinks that we are on Linux and hence we don't need to do this by default.

A workaround when using Ninja is setting CMAKE_NINJA_FORCE_RESPONSE_FILE, which will do the correct thing. We then have another problem, which is that the absolute paths in the response file are not in "wine format" (beginning with Z:), because they are not intercepted by the wrapper script.

I think I found a solution, and I have been using it without problems so far. Shall I open a PR, or do you see a problem with this approach?

I think this sounds reasonable. Does @huangqinjin have any other opinions here?

I think it would be good if we would have some testing of this new codepath (by setting CMAKE_NINJA_FORCE_RESPONSE_FILE), perhaps it's enough to set that in test/test-cmake.sh?

Ideally I would maybe use something else than perl for the runtime rewriting, if it could be done with sed instead, that would feel more right to me, though. (Try to stick to the subset of sed that works identically across GNU and macOS.)

@huangqinjin
Copy link
Contributor

I have some concern.

  1. The file path handling in command line options is not easy (and not exhaustive, no doubt) as you see
    while [ $# -gt 0 ]; do
    a=$1
    case $a in
    [-/][A-Za-z]/*)
    opt=${a:0:2}
    path=${a:2}
    # Rewrite options like -I/absolute/path into -Iz:/absolute/path.
    # This is needed to avoid what seems like a cl.exe/Wine bug combination
    # in some very rare cases, see https://bugs.winehq.org/show_bug.cgi?id=55200
    # for details. In those rare cases, cl.exe fails to find includes in
    # some directories specified with -I/absolute/path but does find them if
    # they have been specified as -Iz:/absolute/path.
    if [ -d "$(dirname "$path")" ] && [ "$(dirname "$path")" != "/" ]; then
    a=${opt}z:$path
    fi
    ;;
    [-/][A-Za-z][A-Za-z]/*)
    opt=${a:0:3}
    path=${a:3}
    # Rewrite options like -Fo/absolute/path into -Foz:/absolute/path.
    # This doesn't seem to be strictly needed for any known case at the moment, but
    # might have been needed with some version of MSVC or Wine earlier.
    if [ -d "$(dirname "$path")" ] && [ "$(dirname "$path")" != "/" ]; then
    a=${opt}z:$path
    fi
    ;;
    /*)
    # Rewrite options like /absolute/path into z:/absolute/path.
    # This is essential for disambiguating e.g. /home/user/file from the
    # tool option /h with the value ome/user/file.
    if [ -d "$(dirname "$a")" ] && [ "$(dirname "$a")" != "/" ]; then
    a=z:$a
    fi
    ;;
    *)
    ;;
    esac
    ARGS+=("$a")
    shift
    done
    CMAKE_NINJA_FORCE_RESPONSE_FILE=TRUE makes cl.exe use @file as well as link.exe. So the best solution is to unify the handling of command line and response file.
  2. User can also provide a @file manually without using CMake. So I think modifying @file inplace is not a good idea.
  3. In case of CMake+Ninja, @file is generated by ninja on the fly and will be removed (unless -d keeprsp is passed to ninja) after command finishes. So it is okay to modify it. As I know those generated @file are under CMakeFiles, so matching @*CMakeFiles*.rsp is better in my opinion if we don't want to handle case 1 and 2 now.
  4. I also prefer sed if we now just want a simple solution to start dealing with @file.

@mstorsjo
Copy link
Owner

@huangqinjin Thanks for the valuable input here!

@marcoesposito1988
Copy link
Author

thanks @huangqinjin, really good feedback! I would then start working on points 3 and 4, if that makes sense for you all

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

No branches or pull requests

3 participants