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

[xerces-c] fixed issue #9654 #9702

Merged
merged 1 commit into from
Jan 29, 2020
Merged

[xerces-c] fixed issue #9654 #9702

merged 1 commit into from
Jan 29, 2020

Conversation

mitza-oci
Copy link
Contributor

Using first attempt and review suggestions from #9655

Describe the pull request

@mitza-oci mitza-oci requested a review from NancyLi1013 January 15, 2020 18:41
@msftclas
Copy link

msftclas commented Jan 15, 2020

CLA assistant check
All CLA requirements met.

file(COPY ${SOURCE_PATH}/LICENSE DESTINATION ${CURRENT_PACKAGES_DIR}/share/xerces-c)
file(RENAME ${CURRENT_PACKAGES_DIR}/share/xerces-c/LICENSE ${CURRENT_PACKAGES_DIR}/share/xerces-c/copyright)
# Handle copyright
file(INSTALL ${SOURCE_PATH}/LICENSE DESTINATION ${CURRENT_PACKAGES_DIR}/share/xerces-c RENAME copyright)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please update
file(INSTALL ${SOURCE_PATH}/LICENSE DESTINATION ${CURRENT_PACKAGES_DIR}/share/xerces-c RENAME copyright)
as
file(INSTALL ${SOURCE_PATH}/LICENSE DESTINATION ${CURRENT_PACKAGES_DIR}/share/${PORT} RENAME copyright)?

@NancyLi1013
Copy link
Contributor

Hi @mitza-oci thanks for this PR.
Could you also bump the version in ports/xerces-c/CONTROL file?

@NancyLi1013
Copy link
Contributor

Have existed PR #9655.

@mitza-oci mitza-oci requested a review from NancyLi1013 January 16, 2020 16:08
@NancyLi1013
Copy link
Contributor

NancyLi1013 commented Jan 17, 2020

It seems that there is something wrong with osx pipeline.
We will go further investigation about the regressions.

@NancyLi1013
Copy link
Contributor

Related #9726.

@mitza-oci
Copy link
Contributor Author

It seems that there is something wrong with osx pipeline.
We will go further investigation about the regressions.

Is there anything left to do before merging this? vcpkg's xerces-c has been broken for some time and our downstream project's CI depends on it.

@NancyLi1013
Copy link
Contributor

xalan-c failed on osx pipeline now.
Could you please take a look and try to fix the regression caused by xalan-c?
install-x64-osx-dbg-out .log

@mitza-oci
Copy link
Contributor Author

xalan-c failed on osx pipeline now.
Could you please take a look and try to fix the regression caused by xalan-c?
install-x64-osx-dbg-out .log

How is it working on master? Unless there is a cached download? xerces-c cannot be downloaded from the URL used:

-- Downloading https://github.com/apache/xerces-c/archive/Xerces-C_3_2_2.tar.gz... Failed. Status: 22;"HTTP response code said error"
CMake Error at scripts/cmake/vcpkg_download_distfile.cmake:175 (message):

      Failed to download file.

Because xerces-c has broken on master for a while, we can't determine if this failure on Apple macOS is new to this PR (shouldn't be).

@NancyLi1013
Copy link
Contributor

Hi @mitza-oci
Since you have changed the URL for xerces-c , it can be downloaded and installed successfully.
So where did you get the above error info?
The current regression is caused by xalan-c instead of xerces-c.
Maybe we should pay attention to xalan-c and try to fix it.

@mitza-oci
Copy link
Contributor Author

Hi @mitza-oci
Since you have changed the URL for xerces-c , it can be downloaded and installed successfully.
So where did you get the above error info?

The above is for master, not my branch. When using master, xerces-c cannot be downloaded. So the CI build indicating "success" on master is not accurate -- my guess is that the CI is using a cached download.

The current regression is caused by xalan-c instead of xerces-c.
Maybe we should pay attention to xalan-c and try to fix it.

I can't reproduce the error locally and I'm not set up to use vcpkg on Apple macOS. This PR as-is restores vcpkg's xerces-c package to a useable state. Someone who knows the macOS-specific code in xalan/xerces should continue to fix any remaining errors.

This is what I see on macOS, vcpkg fails to bootstrap:

.../vcpkg/scripts/bootstrap.sh: line 219: [: Configured with: --prefix=/Applications/Xcode: integer expression expected
-- The C compiler identification is AppleClang 11.0.0.11000033
-- The CXX compiler identification is AppleClang 11.0.0.11000033
-- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc
-- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /Applications/Xcode.app/Contents/Developer/usr/bin/g++
-- Check for working CXX compiler: /Applications/Xcode.app/Contents/Developer/usr/bin/g++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Looking for pthread_create
-- Looking for pthread_create - found
-- Found Threads: TRUE
-- Performing Test USES_LIBSTDCXX
-- Performing Test USES_LIBSTDCXX - Failed
-- Performing Test USES_LIBCXX
-- Performing Test USES_LIBCXX - Success
-- Configuring done
-- Generating done
-- Build files have been written to: .../vcpkg/toolsrc/build.rel
[0/2] Re-checking globbed directories...
[1/67] Building CXX object CMakeFiles/vcpkg.dir/src/vcpkg.cpp.o
FAILED: CMakeFiles/vcpkg.dir/src/vcpkg.cpp.o
/Applications/Xcode.app/Contents/Developer/usr/bin/g++  -DDISABLE_METRICS=0 -I../include -O3 -DNDEBUG -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -mmacosx-version-min=10.14   -std=c++1z -MD -MT CMakeFiles/vcpkg.dir/src/vcpkg.cpp.o -MF CMakeFiles/vcpkg.dir/src/vcpkg.cpp.o.d -o CMakeFiles/vcpkg.dir/src/vcpkg.cpp.o -c ../src/vcpkg.cpp
In file included from ../src/vcpkg.cpp:24:
../include/vcpkg/base/files.h:57:41: error: 'file_status' is unavailable: introduced in macOS 10.15
    struct file_status : private stdfs::file_status

I don't see anything in vcpkg documentation indicating the minimum macOS version required.

@mitza-oci
Copy link
Contributor Author

Looks like the issue was already fixed in #9726 which made almost the exact same change to xerces-c/portfile.cmake. That one has HEAD_REF trunk which I'm not sure will work if someone tries that configuration.

@NancyLi1013
Copy link
Contributor

@mitza-oci
Could you please resolve conflicts about this PR?

@mitza-oci
Copy link
Contributor Author

The package writes to both share/xercesc and share/xerces-c. The license file has to be in xerces-c, should the other files move there as well?

@NancyLi1013
Copy link
Contributor

The *.cmake files such as *target.cmake, *config.cmake should be in share/xercesc, which is decided by TARGET_PATH in vcpkg_fixup_cmake_targets(). Other files such as license, doc, usage should be in share/${PORT} (here is xercesc-c).

@mitza-oci
Copy link
Contributor Author

The *.cmake files such as *target.cmake, *config.cmake should be in share/xercesc, which is decided by TARGET_PATH in vcpkg_fixup_cmake_targets(). Other files such as license, doc, usage should be in share/${PORT} (here is xercesc-c).

I'm seeing vcpkg-cmake-wrapper.cmake in xerces-c. Is that correct?

@mitza-oci
Copy link
Contributor Author

I'm seeing vcpkg-cmake-wrapper.cmake in xerces-c. Is that correct?

Looks like that was a local change, which I reverted.

@mitza-oci
Copy link
Contributor Author

Looks like the error in vcpkg-Linux-PR-test was not related to the change in this PR.

@mitza-oci mitza-oci requested a review from JackBoosY January 27, 2020 20:15
@vicroms
Copy link
Member

vicroms commented Jan 29, 2020

You're right, I'll trigger a rebuild

@vicroms vicroms added info:reviewed Pull Request changes follow basic guidelines and removed waiting for response labels Jan 29, 2020
@mitza-oci
Copy link
Contributor Author

You're right, I'll trigger a rebuild

Thank you, looks like it worked.

@vicroms vicroms merged commit b5b21df into microsoft:master Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants