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

New minimum MacOS version logic causes build error #900

Closed
helixbass opened this issue Nov 13, 2023 · 4 comments · Fixed by #901
Closed

New minimum MacOS version logic causes build error #900

helixbass opened this issue Nov 13, 2023 · 4 comments · Fixed by #901

Comments

@helixbass
Copy link

Hi,
I have a somewhat fuzzy grasp of the problem but think I've tracked this down to being an issue introduced by #848

The problem I'm running into is that when trying to build a Rust project on my Mac (Intel, macos 12.6.9) that uses a build script + cc to build a C++ file it is failing on not being able to find libstdc++. Which I guess is because it "should" be looking for libc++ instead?

For example tree-sitter-ruby, or this test repo

These fail when running cargo build with something like:

  running: "c++" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "-m64" "--target=x86_64-apple-darwin" "-mmacosx-version-min=10.7" "-I" "src" "-Wall" "-Wextra" "-o" "/Users/jrosse/prj/tree-sitter-lint-plugin-eslint-builtin/.tree-sitter-lint/tree-si
tter-lint-local/target/release/build/tree-sitter-ruby-3ed10c8d09b2beda/out/src/scanner.o" "-c" "src/scanner.cc"
  cargo:warning=clang: warning: include path for libstdc++ headers not found; pass '-stdlib=libc++' on the command line to use the libc++ standard library instead [-Wstdlibcxx-not-found]

  cargo:warning=src/scanner.cc:4:10: fatal error: 'cstring' file not found

So my rough analysis is that:

I just started running into this because cc@1.0.84 just got released, which includes #848

#848 appears to introduce the use of that macosx-version-min flag. Then #872 appears to try and change that logic to force a minimum macosx-version-min of 10.9 when you're compiling C++

But it appears that the fix from #872 isn't "kicking in" (and so per above logging, it is using macosx-version-min=10.7, which I guess causes it to use libstdc++ instead of libc++ or something?) because rustc_provided_target() (which runs rustc --target whatever --print deployment-target) is returning 10.7

So I'm guessing the fix here is to refine that #872 logic to eg also force 10.9 when rustc_provided_target() returns something less than 10.9 (eg 10.7)?

@Riolku
Copy link

Riolku commented Nov 13, 2023

In case anyone wants a MWE:

[package]
name = "testing"
version = "0.1.0"
edition = "2021"

[dependencies]

[build-dependencies]
cc = "1.0"
// build.rs

fn main() {
    cc::Build::new()
        .cpp(true)
        .cpp_link_stdlib("c++")
        .file("foo.cpp")
        .compile("foo");
}
// foo.cpp
#include <iostream>

int main() {
	std::cout << "Hello, World!" << std::endl;
}

.cpp_link_stdlib("c++") doesn't seem to help.

Downgrading via cargo update -p cc --precise "1.0.83" fixes the issue temporarily.

@thomcc
Copy link
Member

thomcc commented Nov 14, 2023

Yeah, hmm. I guess when compiling C++ we should not respect what rustc --print deployment-target returns if it's less than 10.9.

@thomcc
Copy link
Member

thomcc commented Nov 14, 2023

I'm away from home so I can't do a good patch for this (which should probably be version cmp + bound to release deployment target that releases libc++), but I could do a hacky check for the problematic version (which in practice should basically always work).

That said, I chatted with @BlackHoleFox who says they'll do it. If they don't get to it by this evening, I'll do the hacky thing as a stop-gap, and then follow up with a better one later. Either way, I'll get a fix out for this this evening or tomorrow morning.

Note that another fix for this is export MACOSX_DEPLOYMENT_TARGET=10.9, although waiting for the next stable release of rustc, which bumps the mac min version, and will cause --print deployment-target to start returning 10.12, would also work. That release should be... tomorrow, by my watch 😅1

Footnotes

  1. cc supports more than the latest version of Rust, so this must be fixed either way.

@BlackHoleFox
Copy link
Contributor

Apologies for busting this unintentionally everyone.

@thomcc #901 should fix the regression.

Riolku added a commit to kuzudb/kuzu that referenced this issue Nov 16, 2023
In the Java API, we had a bug where we take ownership of and free
parameters passed into executeWithParams.

Inspecting the method itself, it was taking a shared_ptr, but then
performing a deep copy, which is nonsense. Instead, we should take a
unique_ptr, since we need to copy the parameters to guarantee that they
are not modified for the duration of the query.

This commit also fixes three other issues. First, the Java tests weren't running
any tests from ConnectionTest.java, which is why we didn't observe this
bug. Additionally, the constructor of KuzuConnection uses an assertion,
but assertions are disabled by default, which causes our tests to fail
(and if the assertion is skipped, we segfault).

Also, since rust-lang/cc-rs#900 has been
fixed, we can remove the version pinning of `cc` on MacOS.
Riolku added a commit to kuzudb/kuzu that referenced this issue Nov 16, 2023
In the Java API, we had a bug where we take ownership of and free
parameters passed into executeWithParams.

Inspecting the method itself, it was taking a shared_ptr, but then
performing a deep copy, which is nonsense. Instead, we should take a
unique_ptr, since we need to copy the parameters to guarantee that they
are not modified for the duration of the query.

This commit also fixes three other issues. First, the Java tests weren't running
any tests from ConnectionTest.java, which is why we didn't observe this
bug. Additionally, the constructor of KuzuConnection uses an assertion,
but assertions are disabled by default, which causes our tests to fail
(and if the assertion is skipped, we segfault).

Also, since rust-lang/cc-rs#900 has been
fixed, we can remove the version pinning of `cc` on MacOS.
Riolku added a commit to kuzudb/kuzu that referenced this issue Nov 16, 2023
In the Java API, we had a bug where we take ownership of and free
parameters passed into executeWithParams.

Inspecting the method itself, it was taking a shared_ptr, but then
performing a deep copy, which is nonsense. Instead, we should take a
unique_ptr, since we need to copy the parameters to guarantee that they
are not modified for the duration of the query.

This commit also fixes three other issues. First, the Java tests weren't running
any tests from ConnectionTest.java, which is why we didn't observe this
bug. Additionally, the constructor of KuzuConnection uses an assertion,
but assertions are disabled by default, which causes our tests to fail
(and if the assertion is skipped, we segfault).

Also, since rust-lang/cc-rs#900 has been
fixed, we can remove the version pinning of `cc` on MacOS.
Riolku added a commit to kuzudb/kuzu that referenced this issue Nov 16, 2023
In the Java API, we had a bug where we take ownership of and free
parameters passed into executeWithParams.

Inspecting the method itself, it was taking a shared_ptr, but then
performing a deep copy, which is nonsense. Instead, we should take a
unique_ptr, since we need to copy the parameters to guarantee that they
are not modified for the duration of the query.

This commit also fixes three other issues. First, the Java tests weren't running
any tests from ConnectionTest.java, which is why we didn't observe this
bug. Additionally, the constructor of KuzuConnection uses an assertion,
but assertions are disabled by default, which causes our tests to fail
(and if the assertion is skipped, we segfault).

Also, since rust-lang/cc-rs#900 has been
fixed, we can remove the version pinning of `cc` on MacOS.
Riolku added a commit to kuzudb/kuzu that referenced this issue Nov 17, 2023
In the Java API, we had a bug where we take ownership of and free
parameters passed into executeWithParams.

Inspecting the method itself, it was taking a shared_ptr, but then
performing a deep copy, which is nonsense. Instead, we should take a
unique_ptr, since we need to copy the parameters to guarantee that they
are not modified for the duration of the query.

This commit also fixes three other issues. First, the Java tests weren't running
any tests from ConnectionTest.java, which is why we didn't observe this
bug. Additionally, the constructor of KuzuConnection uses an assertion,
but assertions are disabled by default, which causes our tests to fail
(and if the assertion is skipped, we segfault).

Also, since rust-lang/cc-rs#900 has been
fixed, we can remove the version pinning of `cc` on MacOS.
Riolku added a commit to kuzudb/kuzu that referenced this issue Nov 17, 2023
In the Java API, we had a bug where we take ownership of and free
parameters passed into executeWithParams.

Inspecting the method itself, it was taking a shared_ptr, but then
performing a deep copy, which is nonsense. Instead, we should take a
unique_ptr, since we need to copy the parameters to guarantee that they
are not modified for the duration of the query.

This commit also fixes three other issues. First, the Java tests weren't running
any tests from ConnectionTest.java, which is why we didn't observe this
bug. Additionally, the constructor of KuzuConnection uses an assertion,
but assertions are disabled by default, which causes our tests to fail
(and if the assertion is skipped, we segfault).

Also, since rust-lang/cc-rs#900 has been
fixed, we can remove the version pinning of `cc` on MacOS.
Riolku added a commit to kuzudb/kuzu that referenced this issue Nov 17, 2023
In the Java API, we had a bug where we take ownership of and free
parameters passed into executeWithParams.

Inspecting the method itself, it was taking a shared_ptr, but then
performing a deep copy, which is nonsense. Instead, we should take a
unique_ptr, since we need to copy the parameters to guarantee that they
are not modified for the duration of the query.

This commit also fixes three other issues. First, the Java tests weren't running
any tests from ConnectionTest.java, which is why we didn't observe this
bug. Additionally, the constructor of KuzuConnection uses an assertion,
but assertions are disabled by default, which causes our tests to fail
(and if the assertion is skipped, we segfault).

Also, since rust-lang/cc-rs#900 has been
fixed, we can remove the version pinning of `cc` on MacOS.
Riolku added a commit to kuzudb/kuzu that referenced this issue Nov 17, 2023
In the Java API, we had a bug where we take ownership of and free
parameters passed into executeWithParams.

Inspecting the method itself, it was taking a shared_ptr, but then
performing a deep copy, which is nonsense. Instead, we should take a
unique_ptr, since we need to copy the parameters to guarantee that they
are not modified for the duration of the query.

This commit also fixes three other issues. First, the Java tests weren't running
any tests from ConnectionTest.java, which is why we didn't observe this
bug. Additionally, the constructor of KuzuConnection uses an assertion,
but assertions are disabled by default, which causes our tests to fail
(and if the assertion is skipped, we segfault).

Also, since rust-lang/cc-rs#900 has been
fixed, we can remove the version pinning of `cc` on MacOS.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants