Skip to content

Commit

Permalink
_build.sh: add a second llvm-windres workaround [ci skip]
Browse files Browse the repository at this point in the history 
With llvm-15, llvm-windres-15 is present, but the Windows Resource
woes are not over: In this case we're using a suffixed llvm, where
clang is called clang-15. llvm-windres is not expecting that, tries
to locate `clang` without suffix, and fails.

Our workaround is to symlink windres to a path we control, and add
another symlink called `clang` (to `clang-15`) on the same path.
This makes windres find our suffixed clang and use it.

Alternatives: 1. `clang` alias could be put in PATH, but that's more
intrusive. 2. windres allows to pass the name of the preprocessor
we'd like it to use, but I could not make this work. It also needs
manually passing all low-level preprocessor arguments from scratch.
3. fallback to binutils windres. Also could not make it work.

Links:
https://reviews.llvm.org/D100755
https://github.com/llvm/llvm-project/blob/5bcb4c4da99c443fb880d408e5ff4e9b305bbb77/llvm/tools/llvm-rc/llvm-rc.cpp#L137
msys2/MINGW-packages#8736

/cc @mstorsjo
  • Loading branch information
@vszakats
vszakats committed Mar 23, 2023
1 parent 1bd2746 commit caaae17
Showing 1 changed file with 26 additions and 8 deletions.
34 changes: 26 additions & 8 deletions _build.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -663,14 +663,32 @@ build_single_target() {
export RC="${_BINUTILS_PREFIX}windres${_BINUTILS_SUFFIX}"
if [ "${_CC}" = 'llvm' ] && \
[ "${_TOOLCHAIN}" != 'llvm-mingw' ] && \
[ "${_OS}" = 'linux' ] && \
! command -v "${RC}" >/dev/null 2>&1 && \
[ -x "/usr/bin/${_BINUTILS_PREFIX}rc${_BINUTILS_SUFFIX}" ]; then
# FIXME: llvm-windres alias (to llvm-rc) may be missing from llvm.
# Workaround: Create an alias and use that.
# https://packages.debian.org/testing/amd64/llvm/filelist
RC="$(pwd)/${RC}"
ln -s -f "/usr/bin/${_BINUTILS_PREFIX}rc${_BINUTILS_SUFFIX}" "${RC}"
[ "${_OS}" = 'linux' ]; then
if ! command -v "${RC}" >/dev/null 2>&1 && \
[ -x "/usr/bin/${_BINUTILS_PREFIX}rc${_BINUTILS_SUFFIX}" ]; then
# FIXME: llvm-windres alias (to llvm-rc) may be missing from llvm.
# Workaround: Create an alias and use that.
# https://packages.debian.org/testing/amd64/llvm/filelist
RC="$(pwd)/${RC}"
ln -s -f "/usr/bin/${_BINUTILS_PREFIX}rc${_BINUTILS_SUFFIX}" "${RC}"
elif [ -n "${_BINUTILS_SUFFIX}" ]; then
# FIXME: llvm-windres present, but unable to find its clang counterpart
# when suffixed:
# llvm-windres-15 -O coff --target=pe-x86-64 -I../include -i libcurl.rc -o x86_64-w64-windows-gnu/libcurl.res
# llvm-rc: Unable to find clang, skipping preprocessing.
# Pass -no-cpp to disable preprocessing. This will be an error in the future.
# (the final name of that option is -no-preprocess, though we do
# need preprocessing here.)
# https://reviews.llvm.org/D100755
# https://github.com/llvm/llvm-project/blob/main/llvm/tools/llvm-rc/llvm-rc.cpp
# https://github.com/msys2/MINGW-packages/discussions/8736
RC="$(pwd)/${RC}"
ln -s -f "/usr/bin/${_BINUTILS_PREFIX}rc${_BINUTILS_SUFFIX}" "${RC}"
# llvm-windres/llvm-rc wants to find clang on the same path as itself
# (or in PATH), with the hard-wired name of clang (or <TRIPLIET>-clang,
# or clang-cl). Workaround: create an alias for it:
ln -s -f "/usr/bin/clang${_CCSUFFIX}" "$(pwd)/clang"
fi
fi
export AR="${_BINUTILS_PREFIX}ar${_BINUTILS_SUFFIX}"
export NM="${_BINUTILS_PREFIX}nm${_BINUTILS_SUFFIX}"
Expand Down

16 comments on commit caaae17

@mstorsjo
Copy link

Choose a reason for hiding this comment

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

Thanks for looping me in. About the alternatives; 2, as you say, windres does have an option for specifying the tool to use for preprocessing, but the option is kinda weird. The option doesn't only set the name of the executable, but the default value of the option was something like $CC -E -xc -DRC_INVOKED (i.e. a string with spaces, split into multiple arguments passed to the preprocessing process). However, in 2021, GNU windres changed their interpretation of this option, into the option just specifying the path of $CC - breaking a couple user projects that used the option according to the previous documentation. LLVM windres hasn't adapted to this change.

Anyway, for your actual problem - would it help if we'd extend the search for clang binaries, in https://github.com/llvm/llvm-project/blob/llvmorg-16.0.0/llvm/tools/llvm-rc/llvm-rc.cpp#L143, into including clang-<major> too? There's still potential for ending up in situations that are problematic to resolve, but the default behaviour would cover more setups.

@vszakats
Copy link
Member Author

@vszakats vszakats commented on caaae17 Mar 23, 2023 •
edited

Choose a reason for hiding this comment

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

@mstorsjo Me thanks for jumping in! For this particular case, looking up
clang-<major> on the same path as windres itself 1, would fix it indeed.
It also looks like a fairly universal solution.

As for the preprocessor override, I'd expected it to be an override for the
PP tool only, while keeping all built-in options intact, though I understand
other situations might need a full override. What could help here, is a verbose
option to display the preprocessor command as executed by windres.
Without it, it was difficult to find out what is happening.

What might also work is preprocessing the .rc manually and passing the
result to windres with preprocessing disabled. But that can also get a little
messy.

A tiny side-issue is that the error message suggests using -no-cpp, while
the final name of the option is -no-preprocess.

Thanks again for looking into this!

Footnotes

  1. Relevant filelists from Debian:
    https://packages.debian.org/testing/amd64/llvm-15/filelist
    https://packages.debian.org/testing/amd64/clang-15/filelist ↩

@mstorsjo
Copy link

Choose a reason for hiding this comment

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

Yes, it would be much more convenient if the option would point out only the executable to use for preprocessing - but the GNU windres option was previously both consistently documented and behaving in this non-obvious way, and some projects had used that option in that form. GNU binutils later changed its behaviour and updated the documentation - not acknowledging that this actually was a breaking change for projects that had used it according to the old documentation, but argued that this was the original intent of the option all along. This means that for third party projects, unless they can limit themselves to e.g. only recent versions of GNU windres, they can't really use this option at all. For llvm-windres, I haven't followed along with this breaking interface change.

If passing -v or --verbose to the llvm-windres tool, it should print out the command that it tries to execute. (It also dumps a lot of info about the resource data. There's also a mostly undocumented option -### which makes it print the command it would have executed, but exit instead of executing it - but that's mostly meant to be used for testing.)

Thanks for the note about the misspelled option reference, I'll try to get that fixed.

@vszakats
Copy link
Member Author

@vszakats vszakats commented on caaae17 Mar 23, 2023 •
edited

Choose a reason for hiding this comment

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

Oops, thanks for mentioning -v, of course I failed to spot it. Also for -###
and the GNU windres background. I can kind of understand all parties here,
with definitely no easy answers. Perhaps the best is to stay on the simplest,
common road as a user. That said, hopefully GNU windres will learn from
llvm-windres to accept a triplet for its --target= option.

Can confirm: the two debug options are super useful. One just needs to use them :)

@mstorsjo
Copy link

Choose a reason for hiding this comment

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

FYI, I think I realized why the option for setting preprocessor didn't work out for you - it was a bug in llvm-rc, see https://reviews.llvm.org/D146793 for a fix for that. https://reviews.llvm.org/D146794 should make it look for clang-<major> too, https://reviews.llvm.org/D146796 fixes the option name in the warning/error message, and https://reviews.llvm.org/D146797 makes the missing preprocessor a hard error instead of trying to fall back on just not running any preprocessing.

@vszakats
Copy link
Member Author

@vszakats vszakats commented on caaae17 Mar 24, 2023 •
edited

Choose a reason for hiding this comment

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

Cool changes, many thanks for these! They look good to me.

One question for D146794: Would it be better to look for clang-<major>
before clang/clang-cl? This way, in case a machine has both suffixed
and non-suffixed LLVM installed, windres would find the matching clang.
Otherwise the suffixed windres would find the non-suffixed clang first,
potentially mixing versions. (Minor downside is that for a non-suffixed
windres, there will be an extra check that fails in the common use-cases
of using a non-suffixed windres.)

Making no PP an error is also fine IMO. Most .rc do need preprocessing,
meaning there would be a error in those cases anyway. An early error
makes the reason clear.

@mstorsjo
Copy link

Choose a reason for hiding this comment

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

Good point about the order for checking; I did think about this a bit (but I'm open for further discussion) The reason I went this way, is because of how these tools are named/labelled within llvm-mingw. There, clang.exe is a small wrapper executable that executes clang-<major>.exe with the options for setting defaults, defaults like -stdlib=libc++ etc, while clang-<major>.exe is the actual regular executable. So for those environments, clang generally gives a more correct/intended experience. But this is only the local convention there...

OTOH, for llvm-mingw builds on actual windows, clang-<major>.exe is something that is built with such defaults hardcoded in the binary too. And in the end, it shouldn't matter much anyway since it will match TargetClang (the one named <targettriple>-clang) before either of those. (And even if it would use a raw uncustomized clang, I don't think .rc files include C++ standard headers that often anyway - the preprocessor is run in C mode too.)

So all in all, I guess that could be reasonable.

For the second search for a suitable clang executable, right below it, where we search in all of $PATH instead of that specific directory, do you think we should include clang-<major> there too?

@vszakats
Copy link
Member Author

@vszakats vszakats commented on caaae17 Mar 24, 2023 •
edited

Choose a reason for hiding this comment

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

On Debian, clang is just a symlink
(./usr/bin/clang -> ../lib/llvm-14/bin/clang), without extra
settings. It's only installed for the default llvm (14). Theoretically
clang might act differently if called with/without a suffix, though
this would be kind of unexpected and confusing, esp. given that
it would only apply to the default llvm version.

I tend to side with looking for the suffixed version first. Based on
our observations. Like you said, for compiling resources, in most
cases it's enough if the compiler can locate Windows headers
(winver.h typically).

As for looking up clang-<major> in PATH. It feels like something
to add only once it turns out to be necessary for an actual use-case.
Can't think of one at the moment!

@mstorsjo
Copy link

Choose a reason for hiding this comment

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

Ok, sounds reasonable, I'll revise the patch that way, then.

@vszakats
Copy link
Member Author

@vszakats vszakats commented on caaae17 Aug 6, 2023 •
edited

Choose a reason for hiding this comment

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

Hi Martin,

I've revisited this today while trying to drop the workaround added back then.
This is possible now that LLVM 16.0.6 is running also on debian-testing. Also
installed as a suffixed package, like v15 was, and the not so happy result is
that the main old issue seems to have remained:

After calling llvm-windres-16, it fails with
llvm-rc: Unable to find clang, skipping preprocessing., like earlier.

According to deb package contents:

  • llvm-windres-16 is /usr/bin/llvm-windres-16.
  • /usr/bin/llvm-windres-16 symlinks to ../lib/llvm-16/bin/llvm-windres,
    that is: /usr/lib/llvm-16/bin/llvm-windres
  • /usr/lib/llvm-16/bin/llvm-windres symlinks to llvm-rc, that is:
    /usr/lib/llvm-16/bin/llvm-rc. Which then fails with missing clang.

The clang to be found is either /usr/bin/clang-16,
/usr/lib/llvm-16/bin/clang, or /usr/lib/llvm-16/bin/clang-16.

PATH is /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin.

Relevant logs:

llvm-windres-16 -O coff  --target=pe-x86-64 -I../include -i libcurl.rc -o x86_64-w64-windows-gnu/libcurl.res
llvm-rc: Unable to find clang, skipping preprocessing.
Pass --no-preprocess to disable preprocessing. This will be an error in the future.
llvm-rc: Error parsing file: expected '-', '~', integer or '(', got RC_VERSION

via: https://ci.appveyor.com/project/curlorg/curl-for-win/builds/47724483#L10136

+ llvm-windres-16 --target=pe-x86-64 -D GCC_WINDRES -I /home/appveyor/projects/curl-for-win/zlib -I /home/appveyor/projects/curl-for-win/zlib/bld -o /home/appveyor/projects/curl-for-win/zlib/bld/zlib1rc.obj -i /home/appveyor/projects/curl-for-win/zlib/win32/zlib1.rc
llvm-rc: Unable to find clang, skipping preprocessing.
Pass --no-preprocess to disable preprocessing. This will be an error in the future.
llvm-rc: Error parsing file: expected fixed VERSIONINFO statement type, got VS_VERSION_INFO

via: https://ci.appveyor.com/project/curlorg/curl-for-win/builds/47723375#L3354

Refs:
https://github.com/llvm/llvm-project/blob/7cbf1a2591520c2491aa35339f227775f4d3adf6/llvm/tools/llvm-rc/llvm-rc.cpp#L137
https://packages.debian.org/trixie/amd64/llvm-16/filelist
https://packages.debian.org/trixie/amd64/clang-16/filelist
https://ftp.debian.org/debian/pool/main/l/llvm-toolchain-16/llvm-16_16.0.6-6_amd64.deb

@mstorsjo
Copy link

Choose a reason for hiding this comment

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

Hi Martin,

I've revisited this today while trying to drop the workaround added back then. This is possible now that LLVM 16.0.6 is running also on debian-testing. Also installed as a suffixed package, like v15 was, and the not so happy result is that the main old issue seems to have remained:

After calling llvm-windres-16, it fails with llvm-rc: Unable to find clang, skipping preprocessing., like earlier.

According to deb package contents:

  • llvm-windres-16 is /usr/bin/llvm-windres-16.
  • /usr/bin/llvm-windres-16 symlinks to ../lib/llvm-16/bin/llvm-windres,
    that is: /usr/lib/llvm-16/bin/llvm-windres
  • /usr/lib/llvm-16/bin/llvm-windres symlinks to llvm-rc, that is:
    /usr/lib/llvm-16/bin/llvm-rc. Which then fails with missing clang.

The clang to be found is either /usr/bin/clang-16, /usr/lib/llvm-16/bin/clang, or /usr/lib/llvm-16/bin/clang-16.

Hmm, that's weird if that didn't get fixed now, as the fix is included in 16.0.1 and newer.

I guess I'll have to set up a debian testing environment and install those packages and look into why it isn't working as inteded.

@mstorsjo
Copy link

Choose a reason for hiding this comment

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

I had a look, and the issue is rather silly. In llvm/llvm-project@282744a I'm looking for clang-<version> in the directory pointed out by llvm::sys::path::parent_path(Argv0). I was pretty sure that Argv0, coming from the Argv[0] that was set up by InitLLVM, always had an absolute path - but this is only the case on Windows, not on Unix - InitLLVM has got a slight behaviour difference in that aspect.

https://reviews.llvm.org/D157241 fixes this issue, tested by dropping in a newly built binary in a debian testing installation.

@vszakats
Copy link
Member Author

@vszakats vszakats commented on caaae17 Aug 6, 2023 •
edited

Choose a reason for hiding this comment

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

Thank you very much for looking into it and your super quick fix; not having
an absolute path there explains it, and it indeed looked correct at first glance.

I'm going to re-test as soon as it lands in the testing env. Expecting it in v17.0.0.

@mstorsjo
Copy link

Choose a reason for hiding this comment

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

Thank you very much for looking into it and your super quick fix; not having an absolute path there explains it, and it indeed looked correct at first glance.

I'm going to re-test as soon as it lands in the testing env. Expecting it in v17.0.0.

FYI, LLVM 17.0.0 stable is out now since 2 weeks, and I see that it's in debian experimental so far.

@vszakats
Copy link
Member Author

Choose a reason for hiding this comment

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

@mstorsjo: Thanks! I've switched to your newest llvm-mingw as soon as you released it. We're on Debian testing. It hasn't appeared there yet, but planning to bump ASAP.

@vszakats
Copy link
Member Author

@vszakats vszakats commented on caaae17 Nov 29, 2023 •
edited

Choose a reason for hiding this comment

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

Debian released llvm-17 for testing today.

I deleted the workarounds, and the builds finished without any issue.

Successful llvm-windres-17 (x86_64) invocations here:
https://ci.appveyor.com/project/curlorg/curl-for-win/builds/48632801?fullLog=true#L6984
https://github.com/curl/curl-for-win/actions/runs/7026817005/job/19120727956#step:3:6993

It will go live with the next lib dependency bump, latest with
the curl release next week.

Thank you so much for your fixes, Martin.

Please sign in to comment.