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

Compiling with Clang on Windows fails at libcurl.rc #7765

Closed
VitalyVaryvdin opened this issue Sep 23, 2021 · 18 comments
Closed

Compiling with Clang on Windows fails at libcurl.rc #7765

VitalyVaryvdin opened this issue Sep 23, 2021 · 18 comments

Comments

@VitalyVaryvdin
Copy link

@VitalyVaryvdin VitalyVaryvdin commented Sep 23, 2021

I did this

Compiling on Windows 10 with Clang 12.0.1. Ninja build files generated by CMake.
Getting error at curl/lib/libcurl.rc.
llvm-rc: error in versioninfo statement (id 1): [build] non-ascii 8-bit codepoint (169) can't occur in a non-unicode string

Removing everything coming after #define RC_VERSION results in successful compilation.

I expected the following

Successful compilation

curl/libcurl version

7.79.1-DEV

operating system

Windows 10.

UPD:
The issue is here VALUE "LegalCopyright", "\xa9 " LIBCURL_COPYRIGHT "\0" /* a9: Copyright symbol */. Removing this results in successful compilation.
To be exact it doesn't like "\xa9 " part.

UPD:
Adding L before the string fixes the issue.
VALUE "LegalCopyright", "\xa9 " LIBCURL_COPYRIGHT "\0" /* a9: Copyright symbol */
Becomes
VALUE "LegalCopyright", L"\xa9 " LIBCURL_COPYRIGHT "\0" /* a9: Copyright symbol */

Copyright symbol is properly displayed in .dll info

Tested under Clang 12.0.1 and MSVC 16.9

@bagder
Copy link
Member

@bagder bagder commented Sep 23, 2021

Is this perhaps what's needed? It seems it assumes a certain dir tree layout that may not be true

--- a/lib/libcurl.rc
+++ b/lib/libcurl.rc
@@ -18,11 +18,11 @@
  * This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY
  * KIND, either express or implied.
  *
  ***************************************************************************/
 #include <winver.h>
-#include "../include/curl/curlver.h"
+#include <curl/curlver.h>
 
 LANGUAGE  0, 0
 
 #define RC_VERSION  LIBCURL_VERSION_MAJOR, LIBCURL_VERSION_MINOR, LIBCURL_VERSION_PATCH, 0
 

@VitalyVaryvdin
Copy link
Author

@VitalyVaryvdin VitalyVaryvdin commented Sep 23, 2021

Nope, doesn't fix the issue.

The issue is here VALUE "LegalCopyright", "\xa9 " LIBCURL_COPYRIGHT "\0" /* a9: Copyright symbol */. Removing this results in successful compilation.

To be exact it doesn't like "\xa9 " part.

@bagder
Copy link
Member

@bagder bagder commented Sep 23, 2021

Ok, that's beyond me. @vszakats, any idea?

@VitalyVaryvdin
Copy link
Author

@VitalyVaryvdin VitalyVaryvdin commented Sep 23, 2021

Not sure if that's correct way to fix that but adding L before the string fixes the issue.

VALUE "LegalCopyright", "\xa9 " LIBCURL_COPYRIGHT "\0" /* a9: Copyright symbol */
Becomes
VALUE "LegalCopyright", L"\xa9 " LIBCURL_COPYRIGHT "\0" /* a9: Copyright symbol */

Copyright symbol is properly displayed in .dll info

Upd: works under MSVC 16.9 as well

@vszakats
Copy link
Member

@vszakats vszakats commented Sep 23, 2021

Codepage requirements/expectations of various compilers and of Windows resource files in general are generally mysterious, so what about replacing that copyright symbol with: (c)? /cc @bagder

Prefixing with L looks interesting, though I have no clue why it works and how portable that is.

UPDATE: L also fixes it in my (non-Windows) LLVM/clang 12.0.1 + mingw-w64 env with command: x86_64-w64-mingw32-windres -c65001. Another thing to try is a literal © and/or the -c65001 (or /C 65001) option.

@VitalyVaryvdin
Copy link
Author

@VitalyVaryvdin VitalyVaryvdin commented Sep 23, 2021

According to https://docs.microsoft.com/en-us/windows/win32/menurc/stringtable-resource

To encode Unicode characters, use an "L" followed by the Unicode characters enclosed by quotes. See the Examples section for an example.

IDS_RUSSIANSTRING L"\x0421\x043f\x0440\x0430\x0432\x043a\x0430"

I might assume this being pretty portable and common practice.

@VitalyVaryvdin VitalyVaryvdin changed the title Compiling with Clang on Windows fails at libcurlc.rc Compiling with Clang on Windows fails at libcurl.rc Sep 23, 2021
@vszakats
Copy link
Member

@vszakats vszakats commented Sep 24, 2021

@VitalyVaryvdin Thanks for the link! It looks good to me. This method may even make any -c codepage options unnecessary.

@bagder
Copy link
Member

@bagder bagder commented Sep 24, 2021

I personally wouldn't mind using the ascii (C) version either. Whatever makes this work better.

@VitalyVaryvdin
Copy link
Author

@VitalyVaryvdin VitalyVaryvdin commented Sep 24, 2021

You're welcome!

Using (C) would probably be better option, since it presumable would never bring any issues.

bagder added a commit that referenced this issue Sep 24, 2021
Reported-by: Vitaly Varyvdin
Assisted-by: Viktor Szakats
Fixes #7765
@dfandrich
Copy link
Collaborator

@dfandrich dfandrich commented Sep 25, 2021

@vszakats
Copy link
Member

@vszakats vszakats commented Sep 25, 2021

So the options are:

  1. Replacing \xa9 with Copyright (C)
  2. Prefixing current string with L
  3. Adding -c65001 option to all resource compiler invocations (this is suppored by clang/msvc/mingw/pocc/icc/bcc), indicating UTF-8 strings in the .rc input. (and possibly change \xa9 to © in libcurl.rc)
  • I think the third option gives the most future-proofing (meaning: any future UTF-8 string will continue to work there without further issues), but it is also the one requiring the most work.
  • Prefixing with L is nice and light, but I'm not sure how to apply it to non-literal strings (ones coming via macros.)
  • Avoiding UTF-8 is the most risk-averse with the cost of a longer and uglier-looking text and no provisions for any future UTF-8 text.

This adds the flag for the Makefile.m32 build-system:

diff --git a/docs/examples/Makefile.m32 b/docs/examples/Makefile.m32
index 45ec8679c..261dc03be 100644
--- a/docs/examples/Makefile.m32
+++ b/docs/examples/Makefile.m32
@@ -118,7 +118,7 @@ CFLAGS += -fno-strict-aliasing
 # comment LDFLAGS below to keep debug info
 LDFLAGS = $(CURL_LDFLAG_EXTRAS) $(CURL_LDFLAG_EXTRAS_EXE) -s
 RC = $(CROSSPREFIX)windres
-RCFLAGS = --include-dir=$(PROOT)/include -O coff
+RCFLAGS = --include-dir=$(PROOT)/include -O coff -c65001
 
 # Set environment var ARCH to your architecture to override autodetection.
 ifndef ARCH
diff --git a/lib/Makefile.m32 b/lib/Makefile.m32
index 9f1f5963d..0cd903e61 100644
--- a/lib/Makefile.m32
+++ b/lib/Makefile.m32
@@ -115,7 +115,7 @@ LDFLAGS = $(CURL_LDFLAG_EXTRAS) $(CURL_LDFLAG_EXTRAS_DLL) -s
 AR = $(CURL_AR)
 RANLIB = $(CURL_RANLIB)
 RC = $(CROSSPREFIX)windres
-RCFLAGS = --include-dir=$(PROOT)/include -DDEBUGBUILD=0 -O coff
+RCFLAGS = --include-dir=$(PROOT)/include -DDEBUGBUILD=0 -O coff -c65001
 STRIP   = $(CROSSPREFIX)strip -g
 
 # Set environment var ARCH to your architecture to override autodetection.
diff --git a/src/Makefile.m32 b/src/Makefile.m32
index 380d264e7..23afbe308 100644
--- a/src/Makefile.m32
+++ b/src/Makefile.m32
@@ -119,7 +119,7 @@ CFLAGS += -fno-strict-aliasing
 LDFLAGS = $(CURL_LDFLAG_EXTRAS) $(CURL_LDFLAG_EXTRAS_EXE) -s
 AR = $(CURL_AR)
 RC = $(CROSSPREFIX)windres
-RCFLAGS = --include-dir=$(PROOT)/include -O coff -DCURL_EMBED_MANIFEST
+RCFLAGS = --include-dir=$(PROOT)/include -O coff -c65001 -DCURL_EMBED_MANIFEST
 STRIP   = $(CROSSPREFIX)strip -g
 
 # We may need these someday

@VitalyVaryvdin
Copy link
Author

@VitalyVaryvdin VitalyVaryvdin commented Sep 25, 2021

The symbol is widely recognized but, under the Berne Convention, is no longer required in most nations to assert a new copyright.
The majority of nations now belong to Berne, and thus do not require copyright notices to obtain copyright.

The symbol itself was just an alternative to "Copyright" word in the documents. So probably going with Copyright (C) or even plain Copyright ... is the most elegant solution.

@bagder
Copy link
Member

@bagder bagder commented Sep 25, 2021

"Copyright (C)" is what we use in hundreds of sources files already so it should be fine I think.

@gvanem
Copy link
Contributor

@gvanem gvanem commented Sep 25, 2021

To be exact it doesn't like "\xa9 " part.

I assume llvm-rc is the default selection done by CMake. But could it not use rc instead?

@bagder
Copy link
Member

@bagder bagder commented Sep 26, 2021

@gvanem what selection is that? Is that even something we control?

@gvanem
Copy link
Contributor

@gvanem gvanem commented Sep 26, 2021

@bagder In man cmake-env-variables, there is a mention of CMAKE_RC_COMPILER. Could perhaps override the built-in RC for clang?

@bagder
Copy link
Member

@bagder bagder commented Sep 26, 2021

It seems that in order to use that variable someone would have to patch lib/CMakeLists.txt to compile libcurl.rc separately from the other sources with that compiler. I think we can go with the ASCII version until someone finds the time and inspiration to do that change...

@bagder bagder closed this in 1ca62bb Sep 26, 2021
@vszakats
Copy link
Member

@vszakats vszakats commented Sep 26, 2021

rc belongs to MSVC, so it's not available in every LLVM build-scenario. Also the MSVC's rc is also sensitive to codepage issues. So, this may create different problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants