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

windows: fail early with a missing windres in autotools #9781

Closed
wants to merge 1 commit into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 22, 2022

windres is not always auto-detected by autotools when building for Windows. When this happened, the build failed with a confusing error due to the empty RC command:

/bin/bash ../libtool --tag=RC --mode=compile  -I../include -DCURL_EMBED_MANIFEST  -i curl.rc -o curl.o
[...]
Usage: /sandbox/curl/libtool [OPTION]... [MODE-ARG]...
Try 'libtool --help' for more information.
libtool:   error: unrecognised option: '-I../include'

Improve this by verifying if RC is set, and fail with a clear error otherwise.

Follow-up to 6de7322

Ref: https://curl.se/mail/lib-2022-10/0049.html
Reported-by: Thomas Glanzmann
Closes #9781

@vszakats vszakats changed the title autotools: improve RC detection autotools: improve RC condition Oct 22, 2022
@vszakats vszakats force-pushed the ac-windres-2 branch 3 times, most recently from c1e722d to cb33284 Compare Oct 22, 2022
vszakats added a commit to vszakats/curl that referenced this pull request Oct 22, 2022
Previous solution assumed that when building a Windows target, `RC` tool
is available and auto-detected by autotools. Sometime this is not true.

Rename and extend the `OS_WINDOWS` conditional to also check if `RC` is
set. Effectively this patch replaces a hard error with just not building
the resources.

The correct fix is (possibly) to set the TRIPLET to a valid one (so that
auto-detection works correctly), and a workaround is to manually point
the `RC` envvar to a working copy of `windres`.

[^1]: https://tg.st/u/ad68a31462958a3ef0c801258bf5e8dde310d9fe062416d37d42f1e996af6bdb.txt

Follow-up to 6de7322

Ref: https://curl.se/mail/lib-2022-10/0049.html

Closes curl#9781
@vszakats vszakats changed the title autotools: improve RC condition autotools: more robust RC condition Oct 22, 2022
@vszakats
Copy link
Member Author

vszakats commented Oct 22, 2022

After iterating through a few options, I'm siding that this is a local configuration issue. Unless there is a valid environment that misses a windres tool (or a valid case/bug where autotools is not detecting it), it seems better to hard-fail in this case, than to fail silently by not compiling the resources.

@jay
Copy link
Member

jay commented Oct 23, 2022

@sithglan's error from the mailing list is below for reference. I think that if I saw this error it would not be immediately apparent to me what is happening. Is it worthwhile to test if rc tool exists in the configuration and fail with "rc required" or something, or do you think this is just specific to him? I haven't heard anything else about this.

make[2]: Entering directory '/srv/kvm/scratch/vlconnect/curl/src'
  CC tool_libinfo.o
/bin/bash ../libtool --tag=RC --mode=compile -I../include -DCURL_EMBED_MANIFEST -i curl.rc -o curl.o
libtool: error: ignoring unknown tag RC
Usage: /srv/kvm/scratch/vlconnect/curl/libtool [OPTION]... [MODE-ARG]...
Try 'libtool --help' for more information.
libtool: error: unrecognised option: '-I../include'
make[2]: *** [Makefile:20990: curl.o] Error 1
make[2]: Leaving directory '/srv/kvm/scratch/vlconnect/curl/src'
make[1]: *** [Makefile:1488: all-recursive] Error 1
make[1]: Leaving directory '/srv/kvm/scratch/vlconnect/curl/src'
make: *** [Makefile:1238: all-recursive] Error 1

@vszakats
Copy link
Member Author

vszakats commented Oct 23, 2022

The way it fails is definitely confusing. I'm not entirely sure what's happening in that env, but the cross-compiler prefix seems wrong (mingw32-). This may be a reason why several autotools auto-detections fail (e.g. checking for mingw32-ar... no). i686-w64-mingw32- is the correct value there, which is also the expected one for x86.

From the log it seems a similar issue happened in the past with OpenSSL, hence the explicit RC= line in the make invocation for it: make -j ${HECS} RC=/usr/bin/i686-w64-mingw32-windres. This is missing from the curl make command, and adding it would fix it.

curl ./configure output already has this:

checking for mingw32-windres... no
checking for windres... no

How could we improve that, to be actually noticable?

@jay
Copy link
Member

jay commented Oct 23, 2022

If it's required then AC_MSG_ERROR but now I'm not so sure because what you describe sounds more like user error. @sithglan

@vszakats vszakats changed the title autotools: more robust RC condition autotools: fail early if windres is missing on Windows Oct 24, 2022
@sithglan
Copy link
Contributor

sithglan commented Oct 24, 2022

@vszakats
Copy link
Member Author

vszakats commented Oct 24, 2022

Thank you Thomas for your logs and report, I'm happy it worked out fine.

I think this patch would improve error handling, so it'd be nice to merge. We're close to the release, and this isn't a showstopper, so maybe after the release would be the best.

@vszakats vszakats changed the title autotools: fail early if windres is missing on Windows windows: fail early with a missing windres in autotools Oct 26, 2022
`windres` is not always auto-detected by autotools when building for
Windows. When this happened, the build failed with a confusing error
due to the empty `RC` command:

```
/bin/bash ../libtool --tag=RC --mode=compile  -I../include -DCURL_EMBED_MANIFEST  -i curl.rc -o curl.o
[...]
Usage: /sandbox/curl/libtool [OPTION]... [MODE-ARG]...
Try 'libtool --help' for more information.
libtool:   error: unrecognised option: '-I../include'
```

Improve this by verifying if `RC` is set, and fail with a clear error
otherwise.

Follow-up to 6de7322

Ref: https://curl.se/mail/lib-2022-10/0049.html
Reported-by: Thomas Glanzmann
Closes curl#9781
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants