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

Don't create empty elements in include path vars #17970

Merged
merged 1 commit into from
May 24, 2021

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented May 17, 2021

  • What does your PR fix?

    This PR fixes a reproducible build error for gettext[tools]:x64-osx when libxml2 was already installed. It may fix a number of similar build quirks with vcpkg_configure_make.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    all (tested on osx), no

  • Does your PR follow the maintainer guide?

    yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    -/-

@dg0yt
Copy link
Contributor Author

dg0yt commented May 17, 2021

Background

I got reproducible failures when I tried to build gettext[tools] on osx with port libxml2 installed. The error pointed to vcpkg's libxml2 where I would have expected to find usage of gettext's embedded libxml2. From running:

gcc -DHAVE_CONFIG_H -I. -I../.././../src/0.21-508c311661/libtextstyle/lib -I..  -I. -I../.././../src/0.21-508c311661/libtextstyle/lib -I.. -I../.././../src/0.21-508c311661/libtextstyle -Iglib -DIN_LIBTEXTSTYLE -DLIBXML_STATIC    -I../.././../src/0.21-508c311661/libtextstyle/lib/libcroco  -DDEPENDS_ON_LIBICONV=1   -fPIC -g -c ../.././../src/0.21-508c311661/libtextstyle/lib/libxml/globals.c

(no reference to vcpkg!) the errors were like:

../.././../src/0.21-508c311661/libtextstyle/lib/libxml/globals.c:367:7: error: illegal initializer (only variables can be initialized)
void *xmlGenericErrorContext = NULL;
      ^
/Users/me/dev/vcpkg/installed/x64-osx/include/libxml/globals.h:360:4: note: expanded from macro 'xmlGenericErrorContext'
(*(__xmlGenericErrorContext()))
   ^
fatal error: too many errors emitted, stopping now [-ferror-limit=]

I looked into vcpkg_configure_cmake and found:

# Used by GCC
set(ENV{C_INCLUDE_PATH} "${_VCPKG_INSTALLED}/include${VCPKG_HOST_PATH_SEPARATOR}${C_INCLUDE_PATH_BACKUP}")

Note that this creates values like:

/Users/me/dev/vcpkg/installed/x64-osx/include:

... including the trailing :, i.e. a separator followed by an empty element. Gcc documentation states that the empty element is treated as the current directory, aka -I.

Now clang detects duplicate . elements:

clang -cc1 version 12.0.5 (clang-1205.0.22.9) default target x86_64-apple-darwin20.4.0
ignoring nonexistent directory "/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/local/include"
ignoring nonexistent directory "/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/Library/Frameworks"
ignoring duplicate directory "."
ignoring duplicate directory "../.././../src/0.21-508c311661/libtextstyle/lib"
ignoring duplicate directory ".."
ignoring duplicate directory "."
  as it is a non-system directory that duplicates a system directory
#include "..." search starts here:
#include <...> search starts here:
 ../.././../src/0.21-508c311661/libtextstyle/lib
 ..
 ../.././../src/0.21-508c311661/libtextstyle
 glib
 ../.././../src/0.21-508c311661/libtextstyle/lib/libcroco
 /usr/local/include
 /Users/kpa/dev/vcpkg/installed/x64-osx/include
 .
 /Library/Developer/CommandLineTools/usr/lib/clang/12.0.5/include
 /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include
 /Library/Developer/CommandLineTools/usr/include
 /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks (framework directory)
End of search list.

... and leaves it only after all explicit entries and the vcpkg include dir.

So that is why libxml/globals.h is not taken from . but from /Users/me/dev/vcpkg/installed/x64-osx/include with libxml2 installed.
And that's probably also the reason for #17892 where the earlier definition is from /usr/local/include.

@autoantwort
Copy link
Contributor

Yes this PR fixes #17892. I have tested it locally. 👍

@dg0yt
Copy link
Contributor Author

dg0yt commented May 18, 2021

x64_osx libmicrohttpd: unrelated, #17653.
x64_uwp libdatrie: unrelated, download error.

@JackBoosY JackBoosY added category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly requires:discussion labels May 18, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented May 18, 2021

More background

clang documentation says that "Empty components in the environment variable are ignored." But this is not true, as can be verified by experimentation and source code inspection.

Reproducer:

mkdir /tmp/MARK-I-OPTION
mkdir /tmp/MARK-ENV-VAR
echo | C_INCLUDE_PATH=/tmp/MARK-ENV-VAR clang -E -Wp,-v - -o /dev/null -I. -I/tmp/MARK-I-OPTION 2> c_include_path-no-colon 
echo | C_INCLUDE_PATH=/tmp/MARK-ENV-VAR: clang -E -Wp,-v - -o /dev/null -I. -I/tmp/MARK-I-OPTION 2> c_include_path-with-colon
diff -U20 c_include_path-no-colon c_include_path-with-colon
echo | CPATH=/tmp/MARK-ENV-VAR clang -E -Wp,-v - -o /dev/null -I. -I/tmp/MARK-I-OPTION 2> cpath-no-colon 
echo | CPATH=/tmp/MARK-ENV-VAR: clang -E -Wp,-v - -o /dev/null -I. -I/tmp/MARK-I-OPTION 2> cpath-with-colon 
diff -U20 cpath-no-colon cpath-with-colon

Output for C_INCLUDE_PATH (somewhat older clang on Linux):

--- c_include_path-no-colon	2021-05-18 19:23:22.718867910 +0200
+++ c_include_path-with-colon	2021-05-18 19:59:06.197846524 +0200
@@ -1,12 +1,14 @@
 clang -cc1 version 8.0.0 based upon LLVM 8.0.0 default target x86_64-pc-linux-gnu
 ignoring nonexistent directory "/include"
+ignoring duplicate directory "."
+  as it is a non-system directory that duplicates a system directory
 #include "..." search starts here:
 #include <...> search starts here:
- .
  /tmp/MARK-I-OPTION
  /tmp/MARK-ENV-VAR
+ .
  /usr/local/include
  /usr/lib/llvm-8/lib/clang/8.0.0/include
  /usr/include/x86_64-linux-gnu
  /usr/include
 End of search list.

Output for CPATH:

--- cpath-no-colon	2021-05-18 19:23:53.654933769 +0200
+++ cpath-with-colon	2021-05-18 19:24:00.914949225 +0200
@@ -1,12 +1,13 @@
 clang -cc1 version 8.0.0 based upon LLVM 8.0.0 default target x86_64-pc-linux-gnu
 ignoring nonexistent directory "/include"
+ignoring duplicate directory "."
 #include "..." search starts here:
 #include <...> search starts here:
  .
  /tmp/MARK-I-OPTION
  /tmp/MARK-ENV-VAR
  /usr/local/include
  /usr/lib/llvm-8/lib/clang/8.0.0/include
  /usr/include/x86_64-linux-gnu
  /usr/include

Reported as https://bugs.llvm.org/show_bug.cgi?id=50398

@JackBoosY
Copy link
Contributor

CMake Error at D:/installed/x64-uwp/share/yasm-tool-helper/yasm-tool-helper.cmake:16 (message):
  Cross-targetting and x64 ports requiring yasm require the x86-windows
  yasm-tool to be available.  Please install yasm-tool:x86-windows first.
Call Stack (most recent call first):
  ports/mpg123/portfile.cmake:37 (yasm_tool_helper)
  scripts/ports.cmake:141 (include)


Error: Building package mpg123:x64-uwp failed with: BUILD_FAILED
Elapsed time for package mpg123:x64-uwp: 1.738 s

I think we should wait for #16478.

@JackBoosY JackBoosY added the depends:different-pr This PR or Issue depends on a PR which has been filed label May 20, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented May 22, 2021

gcc (tested: 7.5) behaves the same.

--- c_include_path-no-colon	2021-05-22 18:59:14.961368312 +0200
+++ c_include_path-with-colon	2021-05-22 18:59:14.965368394 +0200
@@ -1,13 +1,15 @@
 ignoring nonexistent directory "/usr/local/include/x86_64-linux-gnu"
 ignoring nonexistent directory "/usr/lib/gcc/x86_64-linux-gnu/7/../../../../x86_64-linux-gnu/include"
+ignoring duplicate directory "."
+  as it is a non-system directory that duplicates a system directory
 #include "..." search starts here:
 #include <...> search starts here:
- .
  /tmp/MARK-I-OPTION
  /tmp/MARK-ENV-VAR
+ .
  /usr/lib/gcc/x86_64-linux-gnu/7/include
  /usr/local/include
  /usr/lib/gcc/x86_64-linux-gnu/7/include-fixed
  /usr/include/x86_64-linux-gnu
  /usr/include
 End of search list.

@dg0yt
Copy link
Contributor Author

dg0yt commented May 23, 2021

This changes also fixes building gettext[tools] on Linux.

@JackBoosY JackBoosY removed the depends:different-pr This PR or Issue depends on a PR which has been filed label May 24, 2021
@JackBoosY
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JackBoosY
Copy link
Contributor

Ping @Neumann-A for review this PR.

@ras0219-msft ras0219-msft merged commit 44da6b7 into microsoft:master May 24, 2021
@ras0219-msft
Copy link
Contributor

This LGTM, thanks for the fix!

@dg0yt dg0yt deleted the include-path branch May 27, 2021 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants