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

Upgrade cmake to v0.1.50 #124887

Closed
wants to merge 1 commit into from
Closed

Upgrade cmake to v0.1.50 #124887

wants to merge 1 commit into from

Conversation

jfgoog
Copy link
Contributor

@jfgoog jfgoog commented May 8, 2024

This is the latest version. Now that the cc crate has been successfully upgraded, we should upgrade cmake as well, as v0.1.48 is now over 2 years old.

cmake v1.0.49 requires CMAKE_SYSTEM_NAME to be set when cross-compiling, so ensure that we do this for supported Apple OSes.

@rustbot
Copy link
Collaborator

rustbot commented May 8, 2024

r? @albertlarsan68

rustbot has assigned @albertlarsan68.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels May 8, 2024
@onur-ozkan
Copy link
Member

I hope this will be much easier than cc..

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented May 8, 2024

📌 Commit c4135b0 has been approved by onur-ozkan

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 8, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 9, 2024
Upgrade cmake to v0.1.50

This is the latest version. Now that the cc crate has been successfully upgraded, we should upgrade cmake as well, as v0.1.48 is now over 2 years old.
@bors
Copy link
Contributor

bors commented May 9, 2024

⌛ Testing commit c4135b0 with merge 4d27423...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 9, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 9, 2024
@onur-ozkan onur-ozkan added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2024
This is the latest version. Now that the cc crate has been successfully
upgraded, we should upgrade cmake as well, as v0.1.48 is now over 2
years old.

cmake v1.0.49 requires CMAKE_SYSTEM_NAME to be set when cross-compiling,
so ensure that we do this for supported Apple OSes.
@rustbot
Copy link
Collaborator

rustbot commented May 9, 2024

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

@jfgoog
Copy link
Contributor Author

jfgoog commented May 13, 2024

Try CI again?

@onur-ozkan
Copy link
Member

Try CI again?

fyi I missed the PR due to "S-waiting-on-author" label.

@bors r+

@bors
Copy link
Contributor

bors commented May 13, 2024

📌 Commit 4742482 has been approved by onur-ozkan

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 13, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 13, 2024
Upgrade cmake to v0.1.50

This is the latest version. Now that the cc crate has been successfully upgraded, we should upgrade cmake as well, as v0.1.48 is now over 2 years old.

cmake v1.0.49 requires CMAKE_SYSTEM_NAME to be set when cross-compiling, so ensure that we do this for supported Apple OSes.
@bors
Copy link
Contributor

bors commented May 13, 2024

⌛ Testing commit 4742482 with merge 9f330de...

@rust-log-analyzer
Copy link
Collaborator

The job dist-apple-various failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
  /usr/local/Cellar/cmake/3.29.2/share/cmake/Modules/CMakeSystemSpecificInitialize.cmake:34 (include)
  CMakeLists.txt:16 (project)


-- Configuring incomplete, errors occurred!
thread 'main' panicked at /Users/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/cmake-0.1.50/src/lib.rs:1098:5:
command did not execute successfully, got: exit status: 1

build script failed, must exit now
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@bors
Copy link
Contributor

bors commented May 13, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 13, 2024
@onur-ozkan onur-ozkan added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 13, 2024
@onur-ozkan
Copy link
Member

@bors rollup=never

@jfgoog
Copy link
Contributor Author

jfgoog commented May 15, 2024

  • cmake v1.0.49 insists that CMAKE_SYSTEM_NAME be set when cross-compiling, and will set it if not provided by the caller.

  • cmake v1.0.50 removes a workaround to for iOS and tvos to prevent an incorrect sysroot from being used from the SDKROOT environment variable (similar to an issue we faced updating the cc crate in Update cc crate for bootstrap to v1.0.97 #122504). The same workaround appears in src/bootstrap/src/core/build_steps/llvm.rs when building LLVM for ios, watchos, tvos, and visionos.

The result of these two changes is that when running cmake, we add "-DCMAKE_SYSTEM_NAME=iOS" and remove "-DCMAKE_OSX_SYSROOT=/" and "-DCMAKE_OSX_DEPLOYMENT_TARGET="

These two seem to interact badly with some built-in cmake logic. On my system, /usr/share/cmake-3.28/Modules/Platform/Darwin-Initialize.cmake has logic based on CMAKE_OSX_SYSROOT:

if(CMAKE_OSX_SYSROOT)
  # Use the existing value without further computation to choose a default.
  set(_CMAKE_OSX_SYSROOT_DEFAULT "${CMAKE_OSX_SYSROOT}")
elseif(NOT "x$ENV{SDKROOT}" STREQUAL "x" AND
        (NOT "x$ENV{SDKROOT}" MATCHES "/" OR IS_DIRECTORY "$ENV{SDKROOT}"))
  # Use the value of SDKROOT from the environment.
  set(_CMAKE_OSX_SYSROOT_DEFAULT "$ENV{SDKROOT}")
elseif(CMAKE_SYSTEM_NAME STREQUAL iOS)
  set(_CMAKE_OSX_SYSROOT_DEFAULT "iphoneos")
elseif(CMAKE_SYSTEM_NAME STREQUAL tvOS)
  set(_CMAKE_OSX_SYSROOT_DEFAULT "appletvos")
# etc.

Then, later on:

# Transform CMAKE_OSX_SYSROOT to absolute path
set(_CMAKE_OSX_SYSROOT_PATH "")
if(CMAKE_OSX_SYSROOT)
  if("x${CMAKE_OSX_SYSROOT}" MATCHES "/")
    # This is a path to the SDK.  Make sure it exists.
    if(NOT IS_DIRECTORY "${CMAKE_OSX_SYSROOT}")
      message(WARNING "Ignoring CMAKE_OSX_SYSROOT value:\n ${CMAKE_OSX_SYSROOT}\n"
        "because the directory does not exist.")
      set(CMAKE_OSX_SYSROOT "")
    endif()
    set(_CMAKE_OSX_SYSROOT_PATH "${CMAKE_OSX_SYSROOT}")
  else()
    _apple_resolve_sdk_path(${CMAKE_OSX_SYSROOT} _sdk_path)
    if(IS_DIRECTORY "${_sdk_path}")
      set(_CMAKE_OSX_SYSROOT_PATH "${_sdk_path}")
      # For non-Xcode generators use the path.
      if(NOT "${CMAKE_GENERATOR}" MATCHES "Xcode")
        set(CMAKE_OSX_SYSROOT "${_CMAKE_OSX_SYSROOT_PATH}")
      endif()
    endif()
  endif()
endif()

This gets triggered from /usr/share/cmake-3.28/Modules/Platform/iOS-Initialize.cmake, which begins:

include(Platform/Darwin-Initialize)

if(NOT _CMAKE_OSX_SYSROOT_PATH MATCHES "/iPhone(OS|Simulator)")
  message(FATAL_ERROR "${CMAKE_OSX_SYSROOT} is not an iOS SDK")
endif()

So what seem to be happening is

  • Setting CMAKE_SYSTEM_NAME (which cmake v1.0.49 guarantees is set) triggers the validation logic in iOS-Initialize.cmake.
  • Then, Darwin-Initialize.cmake tries to determine the SDK from CMAKE_OSX_SYSROOT, or $ENV{SDKROOT} and transform it into a path, but does so incorrectly, and the validation logic fails.

@jfgoog
Copy link
Contributor Author

jfgoog commented May 15, 2024

Note to future someone: Cmake caching makes this one hard to debug. You need to blow away build/aarch64-apple-ios/native/sanitizers between builds.

@jieyouxu
Copy link
Member

@bors r- (manually r- because the tree looks wonky with some PRs that shouldn't be elligble for rollup)

@Dylan-DPC
Copy link
Member

@jfgoog any updates on this? thanks

@jfgoog
Copy link
Contributor Author

jfgoog commented Aug 15, 2024

@jfgoog any updates on this? thanks

I wasn't able to get it to work, and had to move on. I think I put all the stuff I figured out in this CL, so I hope it will be useful to someone else who wants to give it a try.

@alex-semenyuk
Copy link
Member

@Dylan-DPC Should we close PR if author is not interesting in proceeding?

@Dylan-DPC
Copy link
Member

@Dylan-DPC Should we close PR if author is not interesting in proceeding?

@alex-semenyuk yes. Will close this one :)

@Dylan-DPC Dylan-DPC closed this Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants