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

[vcpkg-tool-gn] Build gn from source on 16 KiB page arm64-Linux #39474

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ADKaster
Copy link
Contributor

@ADKaster ADKaster commented Jun 24, 2024

This allows users with Asahi Linux machines to build vcpkg ports that require the gn build tool.

Fixes ##39468

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

This approach... works, but might not be the desired direction? This appears to be the first "host tool" that actually wants to build native code. The way to get a compiler for gn to use is a bit sketchy.

This allows users with Asahi Linux machines to build vcpkg ports that
require the gn build tool.
@ADKaster
Copy link
Contributor Author

@microsoft-github-policy-service agree

@ADKaster ADKaster marked this pull request as ready for review June 24, 2024 00:44
@dg0yt
Copy link
Contributor

dg0yt commented Jun 24, 2024

(community feedback)

  • I would probably put building from source into a separate cmake script.
  • I would not check for pagesize at all. Doing that in script mode is clumsy and duplicates build-related code. A proper implementation would call the vcpkg function to build a test project.
    Simply select build from soure when no matching binary is available.

@LilyWangLL LilyWangLL added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Jun 24, 2024
@ADKaster
Copy link
Contributor Author

ADKaster commented Jun 24, 2024

Simply select build from source when no matching binary is available.

I'm not sure how this approach would fix the linked issue, #39468

The problem there is that while there is a linux-arm64 binary available, it is unusable if the system page size is set to 16 KiB (a kernel config value). So we need a way to detect that the existing linux-arm64 binary will not be usable on this platform, and to build from source instead.

Is there another way to detect this property of the host system at script time that would be better?

I do know that the information is available in the /proc/ filesystem on linux. So if we only want it to be checked for Linux arm64, we could look at /proc in a way like this:

$ grep -i pagesize /proc/self/smaps | sort | uniq
KernelPageSize:        4 kB
MMUPageSize:           4 kB

or via getconf(1)

$ getconf PAGESIZE
4096

@BillyONeal BillyONeal added requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Jun 25, 2024
@dg0yt
Copy link
Contributor

dg0yt commented Jun 25, 2024

Sorry, I didn't pay enought attention. IMO there are three alternatives:

  • a feature (needs discussion; covers other platforms)
  • a triplet variable (easy; covers other platforms)
  • a small build with maintainer functions (still quite easy; but limited platform coverage).

The second option only needs

if(VCPKG_TOOL_GN_FROM_SOURCE) # or similar
   include(build.cmake)
else()
   # do as before
endif()

in the portfile, and

set(VCPKG_TOOL_GN_FROM_SOURCE 1)

in the custom triplet. It could be easily transformed to the first option.

Community feedback. Looking forward to hear from the MS team.

@Neumann-A
Copy link
Contributor

This just runs into the tool ports blocker.

@BillyONeal
Copy link
Member

@AugP
@ras0219-msft
@JavierMatosD
and I

discussed this today.

  1. What makes gn special and lets the other aarch64 things we expect to download work? We aren't saying that we necessarily need a complete solution to everything about this before this can work but we need to understand the problem space to evaluate the correctness of this change. Do we need to look at configuration information from the compiler itself? (For example, extending detect_compiler to tell us more things)

In particular we need to have high confidence that it isn't going to break binary caching. Ideally we also know the end to end story works before we make incremental steps.

  1. Sorry, I didn't pay enought attention. IMO there are three alternatives:

  • a feature (needs discussion; covers other platforms)
  • a triplet variable (easy; covers other platforms)
  • a small build with maintainer functions (still quite easy; but limited platform coverage).

It seems like this needs to be an entirely different 'cpu' because this will affect vcpkg fetch and friends too.

@BillyONeal BillyONeal added the depends:tool-ports Needs tool ports to be a better understood area before proceeding label Jun 28, 2024
@ADKaster
Copy link
Contributor Author

What makes gn special and lets the other aarch64 things we expect to download work?

@BillyONeal

My understanding of the reason that gn has issues on 16 KiB Linux kernels and other tools do not is a combination of factors:

  • the gn binaries that the chromium package sources ship are fully statically linked
  • gn uses the jemalloc allocator. Or at least, the binaries shipped by the chromium package repos do. Jemalloc uses the system page size as a compile time constant. If the system page size and the page size that jemalloc was compiled for do not match at runtime, it is a fatal error.

The other tools such as CMake, ninja, meson, etc that have aarch64 binaries downloaded from a third party do not statically link jemalloc, or another allocator that hard codes the system page size at compile time.

@ADKaster
Copy link
Contributor Author

ADKaster commented Jun 28, 2024

Another alternative could be to create infrastructure for vcpkg to build from source and cache tool binaries on GitHub or another CDN that the upstream doesn't ship.

}
")

# FIXME: Is there a vcpkg approved :tm: try_run replacement?
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need it.

  • Make a project subdir with regular CMakeLists.txt.
  • Use vcpkg_cmake_configure(SOURCE_PATH "${CURRENT_PORT_DIR}/project" ...) - it will pass the actual toolchain settings.
    Make the configuration always succeed, and make it write a result file (aligned via OPTIONS "-DRESULT_FILE=${CURRENT_BUILDTREES_DIR}/result-${TARGET_TRIPLET}.cmake") with the desired information.
  • Load the result file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping @ADKaster for response. Can you address the review suggestions? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @LilyWangLL , sorry for the late reply. I don't think I'll have the capacity to address the comments on this issue.

@BillyONeal BillyONeal removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Sep 13, 2024
@LilyWangLL LilyWangLL marked this pull request as draft October 16, 2024 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist depends:tool-ports Needs tool ports to be a better understood area before proceeding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants