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

Define CMAKE_SYSTEM_NAME on a cross build targeting DragonFly. #113996

Merged

Conversation

inferiorhumanorgans
Copy link

Without CMAKE_SYSTEM_NAME set to the target a cross compile will generally fail. Related to #109170.

@rustbot
Copy link
Collaborator

rustbot commented Jul 24, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ozkanonur (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@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 Jul 24, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 24, 2023

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

@inferiorhumanorgans
Copy link
Author

Given that this list is likely a.) not complete and b.) will grow over time, would it make sense to call splitn on target and turn this into a match statement instead?

@inferiorhumanorgans inferiorhumanorgans changed the title bootstrap: Define CMake platform if DragonFly. Define CMAKE_SYSTEM_NAME on a cross build targeting DragonFly. Jul 24, 2023
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

LGTM after the update of

Change this file to make users of the `download-ci-llvm` configuration download
a new version of LLVM from CI, even if the LLVM submodule hasn’t changed.
Last change is for: https://github.com/rust-lang/rust/pull/112931

@onur-ozkan
Copy link
Member

would it make sense to call splitn on target and turn this into a match statement instead?

I don't think it makes any difference since it's still a string comparison

@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 Jul 24, 2023
@inferiorhumanorgans
Copy link
Author

would it make sense to call splitn on target and turn this into a match statement instead?

I don't think it makes any difference since it's still a string comparison

Ah I was thinking mainly in terms of legibility, however that wouldn't account for platforms where the triplet is actually a quadruplet (e.g. linux-musl or windows-msvc).

Anyways the other thing that comes to mind is printing a warning if CMAKE_SYSTEM_NAME is left undefined e.g.

} else {
    eprintln!("could not determine CMAKE_SYSTEM_NAME from the target `{target}`, build may fail")
}

@onur-ozkan
Copy link
Member

onur-ozkan commented Jul 24, 2023

Anyways the other thing that comes to mind is printing a warning if CMAKE_SYSTEM_NAME is left undefined e.g.

} else {
    eprintln!("could not determine CMAKE_SYSTEM_NAME from the target `{target}`, build may fail")
}

This seems better. Can you add that else block which prints this information with builder.info instead of eprintln? Once this is done with squashing the commits into single one, we can land this :)

CMAKE_SYSTEM_NAME is defined on a cross build if the target is
recognized.  Without this explicit definition cmake will assume that
we're building for the host platform which can bring in unwanted
compiler and linker flags.

Also, add a warning on cross builds with unknown target to aid in
cross builds for future platforms.
@onur-ozkan
Copy link
Member

Thanks a lot!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 25, 2023

📌 Commit a0538db has been approved by ozkanonur

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 Jul 25, 2023
@bors
Copy link
Contributor

bors commented Jul 25, 2023

⌛ Testing commit a0538db with merge 2395c2294bc198f37c162a70f516a6e2b1be2aa6...

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[TIMING] compile::Std { target: x86_64-unknown-linux-gnu, compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu }, crates: [], force_recompile: false } -- 0.129
##[group]Documenting compiler {coverage_test_macros, rustc-main, rustc_abi, rustc_apfloat, rustc_arena, rustc_ast, rustc_ast_lowering, rustc_ast_passes, rustc_ast_pretty, rustc_attr, rustc_baked_icu_data, rustc_borrowck, rustc_builtin_macros, rustc_codegen_llvm, rustc_codegen_ssa, rustc_const_eval, rustc_data_structures, rustc_driver, rustc_driver_impl, rustc_error_codes, rustc_error_messages, rustc_errors, rustc_expand, rustc_feature, rustc_fluent_macro, rustc_fs_util, rustc_graphviz, rustc_hir, rustc_hir_analysis, rustc_hir_pretty, rustc_hir_typeck, rustc_incremental, rustc_index, rustc_infer, rustc_interface, rustc_lexer, rustc_lint, rustc_lint_defs, rustc_llvm, rustc_log, rustc_macros, rustc_metadata, rustc_middle, rustc_mir_build, rustc_mir_dataflow, rustc_mir_transform, rustc_monomorphize, rustc_parse, rustc_parse_format, rustc_passes, rustc_plugin_impl, rustc_privacy, rustc_query_impl, rustc_query_system, rustc_resolve, rustc_serialize, rustc_session, rustc_smir, rustc_span, rustc_symbol_mangling, rustc_target, rustc_trait_selection, rustc_traits, rustc_transmute, rustc_ty_utils, rustc_type_ir} (stage0 -> stage1, x86_64-unknown-linux-gnu)
[TIMING] tool::Rustdoc { compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu } } -- 0.000
##[group]Building LLVM for x86_64-unknown-linux-gnu
thread 'main' panicked at 'nested groups are not supported by GHA!', llvm.rs:275:30
##[endgroup]
Build completed unsuccessfully in 0:00:00
##[group]Clock drift check
  local time: Tue Jul 25 07:30:21 UTC 2023

@bors
Copy link
Contributor

bors commented Jul 25, 2023

💔 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 Jul 25, 2023
@onur-ozkan
Copy link
Member

Seems like a regression from #113514 similar to #113798

@onur-ozkan
Copy link
Member

onur-ozkan commented Jul 26, 2023

a fix PR was merged #114062

@bors retry

@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 Jul 26, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 26, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#101994 (rand: freebsd update, using getrandom.)
 - rust-lang#113930 (Add Param and Bound ty to SMIR)
 - rust-lang#113942 (Squelch a noisy rustc_expand unittest)
 - rust-lang#113996 (Define CMAKE_SYSTEM_NAME on a cross build targeting DragonFly.)
 - rust-lang#114070 (Add `sym::iter_mut` + `sym::as_mut_ptr` for Clippy)
 - rust-lang#114073 (Remove -Z diagnostic-width)
 - rust-lang#114090 (compiletest: remove ci-specific remap-path-prefix)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 998fd94 into rust-lang:master Jul 26, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Jul 26, 2023
@inferiorhumanorgans inferiorhumanorgans deleted the dragonfly-cmake-system-name branch August 1, 2023 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

5 participants