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

Cmake restart fix v2 #101

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Dushistov
Copy link

Extracted from comments that I add to cmake-rs:

// Acording to
// https://gitlab.kitware.com/cmake/cmake/-/issues/18959
// CMake does not support usage of the same build directory for different
// compilers. The problem is that we can not make sure that we use the same compiler
// before running of CMake without CMake's logic duplication (for example consider
// usage of CMAKE_TOOLCHAIN_FILE). Fortunately for us, CMake can detect is
// compiler changed by itself. This is done for interactive CMake's configuration,
// like ccmake/cmake-gui. But after compiler change CMake resets all cached variables.
// So

run cmake twice with parameters specified by user of cmake-rs to make sure that
settings not disappear. Tested on macOS during several weeks. As I found out the problem that I faced in #98 ,
is unrelated to this patch, and related to rust-lang/cc-rs#530 , but I made little changes of the implementation
to make it more robust.

@Dushistov
Copy link
Author

I faced with this problem on macOS, where /usr/bin/clang and /Application/Xcode/.../clang are different binaries,
but have the same version. Plus some tools like to set /usr/bin/cc (that link to /usr/bin/clang) but cmake can not distinguish them.

Dushistov added a commit to Dushistov/couchbase-lite-rust that referenced this pull request Aug 7, 2020
the current release of tokio-tungstenite contains all suitable fixes
add notes about
hyperium/http#422
rust-lang/cmake-rs#96
rust-lang/cmake-rs#101
@Dushistov
Copy link
Author

@alexcrichton , have no time to review, or have some doubts about this PR?

@alexcrichton
Copy link
Member

First off, I want to say I'm sorry for being so glacially slow on this PR, I got too used to this email sitting in my inbox.

Other than that though I'm reading this over and this looks pretty reasonable to me, thanks for sending this PR and doing the work here! Code-wise I was wondering if perhaps run_{configure,build} could share some implementation? They're largely the same except for one needing to run two commands instead of one. Also, the comment on run_and_check_if_need_reconf looks like it may have been cut off?

Again I'm sorry about the pace here, but I'll do my best to be more responsive on this in the future!

@Dushistov Dushistov force-pushed the cmake-restart-fix-v2 branch from 6518357 to b879aac Compare February 12, 2021 13:22
@Dushistov
Copy link
Author

run_{configure,build}

refactor these two into run_cmake_action function

the comment on run_and_check_if_need_reconf looks like it may have been cut off?

Also I removed unfinished sentence in comment

Plus rebase.

@Dushistov
Copy link
Author

Ping @alexcrichton

@Dushistov Dushistov force-pushed the cmake-restart-fix-v2 branch 2 times, most recently from f1ce096 to 7ece076 Compare April 27, 2021 16:06
@Dushistov Dushistov force-pushed the cmake-restart-fix-v2 branch from 7ece076 to 285d900 Compare August 11, 2021 19:03
@Dushistov
Copy link
Author

I rebased my branch, it is again ready to merge @alexcrichton

@alexcrichton
Copy link
Member

Sorry I think I'm gonna be real with myself, I don't have time to maintain this crate any more. I've opened #127.

@Dushistov Dushistov force-pushed the cmake-restart-fix-v2 branch 2 times, most recently from fad72bd to a97e526 Compare August 27, 2021 11:42
@Dushistov Dushistov force-pushed the cmake-restart-fix-v2 branch from a97e526 to 743ddfa Compare November 11, 2021 14:18
@Dushistov
Copy link
Author

Rebase on master.
Ping @alexcrichton and @strega-nil .

@Dushistov Dushistov force-pushed the cmake-restart-fix-v2 branch from 743ddfa to dfa2f23 Compare February 10, 2022 11:35
@tgross35 tgross35 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants