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

[chromium-base] Add new port #12396

Merged
merged 9 commits into from
Sep 6, 2020
Merged

Conversation

vejmartin
Copy link
Contributor

@vejmartin vejmartin commented Jul 13, 2020

Follow-up from #12280

  • What does your PR fix?
    Fixes Add chromium base port #8850.

  • Which triplets are supported/not supported?
    x64-osx, x64-windows, x64-windows-static

  • Have you updated the CI baseline?
    Yes

  • Does your PR follow the maintainer guide?
    Yes

@JackBoosY JackBoosY self-assigned this Jul 13, 2020
@JackBoosY JackBoosY added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jul 13, 2020
@JackBoosY
Copy link
Contributor

For x64-osx, the ci test reported:

ERROR at //build/config/mac/mac_sdk.gni:89:19: Script returned non-zero exit code.
_mac_sdk_result = exec_script(script_name, sdk_info_args, "scope")
                  ^----------
Current dir: /Volumes/data/buildtrees/chromium-base/x64-osx-dbg/
Command: python /Volumes/data/buildtrees/chromium-base/src/25ce732/build/config/mac/sdk_info.py macosx
Returned 1.
stderr:

xcode-select: error: tool 'xcodebuild' requires Xcode, but active developer directory '/Library/Developer/CommandLineTools' is a command line tools instance
Traceback (most recent call last):
  File "/Volumes/data/buildtrees/chromium-base/src/25ce732/build/config/mac/sdk_info.py", line 152, in <module>
    FillXcodeVersion(settings, args.developer_dir)
  File "/Volumes/data/buildtrees/chromium-base/src/25ce732/build/config/mac/sdk_info.py", line 64, in FillXcodeVersion
    lines = subprocess.check_output(['xcodebuild', '-version']).splitlines()
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 223, in check_output
    raise CalledProcessError(retcode, cmd, output=output)
subprocess.CalledProcessError: Command '['xcodebuild', '-version']' returned non-zero exit status 1

See //build/toolchain/mac/BUILD.gn:15:1: whence it was imported.
import("//build/config/mac/mac_sdk.gni")
^--------------------------------------
See //BUILD.gn:1:1: which caused the file to be included.
static_library("chromium-base") {
^--------------------------------

Does this port not support osx currently?

scripts/ci.baseline.txt Outdated Show resolved Hide resolved
@vejmartin vejmartin force-pushed the port/crbase branch 4 times, most recently from 7da0d06 to 9127ba7 Compare July 13, 2020 17:52
@vejmartin vejmartin marked this pull request as ready for review July 13, 2020 17:59
@ras0219-msft
Copy link
Contributor

An initial skim leaves me very impressed, this is an awesome PR! Thank you for avoiding depot_tools :)

I would like to avoid checking in any // Copyright 2006, Google Inc. code if possible, specifically gtest_prod.h and minimizing build_overrides/build.gni as well.

Finally, it would be very nice if vcpkg_find_acquire_program(CLANG) detects the copy of clang/llvm in VS2019 -- I don't know that it doesn't, but this is something I'd like to check.

@MVoz
Copy link
Contributor

MVoz commented Jul 14, 2020

@ras0219-msft

example

msbuild /p:LLVMInstallDir="C:\Program Files (x86)\LLVM" /p:PlatformToolset="ClangCL"

vcpkg_find_acquire_program(CLANG)
get_filename_component(CLANG_DIR "${CLANG}" DIRECTORY)

msbuild /p:LLVMInstallDir="${CLANG_DIR}/.." /p:PlatformToolset="ClangCL"

@vejmartin

only x64 llvm ?

@vejmartin vejmartin force-pushed the port/crbase branch 2 times, most recently from 9c7a249 to 7987d56 Compare July 14, 2020 05:52
@vejmartin
Copy link
Contributor Author

Thank you for the review!

I patched base so that it doesn't include gtest_prod.h and slimmed down build_overrides.
LLVM in Visual Studio 2019 (including the LLVMInstallDir override) should now also be supported as per the docs :)

@MVoz
Copy link
Contributor

MVoz commented Jul 15, 2020

@vejmartin

why not add the x86 version right away ? it will not hurt, but in the future it may be useful
I'm only talking about the script vcpkg_find_acquire_program.cmake , not about the chromium-base port
https://github.com/llvm/llvm-project/releases/download/llvmorg-10.0.0/LLVM-10.0.0-win32.exe

@vejmartin vejmartin force-pushed the port/crbase branch 2 times, most recently from 6f61cc0 to c9173be Compare July 15, 2020 17:28
@vejmartin
Copy link
Contributor Author

@voskrese, the 32-bit version of LLVM should now get selected if the host is 32-bit Windows.

@JackBoosY JackBoosY added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:reviewed Pull Request changes follow basic guidelines labels Jul 16, 2020
@JackBoosY
Copy link
Contributor

Another big step, thanks for this PR!

@iHuahua
Copy link
Contributor

iHuahua commented Jul 20, 2020

什么时候合进master啊?

@iHuahua
Copy link
Contributor

iHuahua commented Jul 22, 2020

@vejmartin does it supported linux?

@vejmartin
Copy link
Contributor Author

@iHuahua, this port only adds support for macOS and Windows (x64) as these are the platforms I can debug locally. I'd expect adding support for Linux would be similar in approach.

@JackBoosY JackBoosY added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Aug 18, 2020
@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Aug 24, 2020
@JackBoosY
Copy link
Contributor

@BillyONeal Ping for merge this PR.

@BillyONeal
Copy link
Member

Tagging requires:discussion to confirm that deploying LLVM like that is OK. I'm 99.9% sure it's fine but want to be safe.

@BillyONeal
Copy link
Member

We discussed this in person today and we're OK with the LLVM deployment. However we aren't sure about the path-suffix part 7ddc48a . Generally speaking all libraries should be under lib or lib/manual-link, so that they can be automatically detected upon use. But base.lib is probably too generic a name. Is it named base.lib in chromium itself?

/cc @ras0219 @ras0219-msft

@JackBoosY JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label Aug 28, 2020
@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BillyONeal
Copy link
Member

Everyone loves when the upstream for the port we use in our end to end tests is down eh?

@iHuahua
Copy link
Contributor

iHuahua commented Sep 5, 2020

@vejmartin hi, supperman, I try to link chromium-base(x64-windows) with Visual Studio 2019, it crash when chromium-base generate string, such as

std::cout << base::GenerateGUID();

And build failed on x64-windows-static with error : /failifmismatch: mismatch detected for '_ITERATOR_DEBUG_LEVEL':

@BillyONeal
Copy link
Member

@iHuahua That means your program needs to be built with the same IDL setting chromium-base uses. vcpkg isn't injecting that so you might need to look upstream.

@vejmartin Thanks for your contribution!

@BillyONeal BillyONeal merged commit f7cd54f into microsoft:master Sep 6, 2020
@vejmartin
Copy link
Contributor Author

@BillyONeal, happy to help 😄

@iHuahua, chromium-base disables iterator debugging, so _ITERATOR_DEBUG_LEVEL=0 needs adding to practically all libraries linked together with chromium-base (the ones that use the C++ Standard Library). To do that for other libraries in vcpkg, you could also add it to the VCPKG_CXX_FLAGS flags in a custom triplet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add chromium base port
7 participants