Skip to content

allowing fallback to clang on freebsd when needed#26185

Merged
bors merged 3 commits into
rust-lang:masterfrom
dhuseby:fixing_freebsd_configure
Jun 24, 2015
Merged

allowing fallback to clang on freebsd when needed#26185
bors merged 3 commits into
rust-lang:masterfrom
dhuseby:fixing_freebsd_configure

Conversation

@dhuseby

@dhuseby dhuseby commented Jun 10, 2015

Copy link
Copy Markdown

On FreeBSD machines without GCC installed, the configure script will now fall back to using clang.

@rust-highfive

Copy link
Copy Markdown
Contributor

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@dhuseby

dhuseby commented Jun 10, 2015

Copy link
Copy Markdown
Author

This fixes #26183.

@wca

wca commented Jun 10, 2015

Copy link
Copy Markdown
Contributor

I'm confused. My test machine doesn't have GCC installed, and the build passes for me with my original change. I thought the configure script already used clang when available, but didn't look at the mechanism since it worked for me.

@wca

wca commented Jun 10, 2015

Copy link
Copy Markdown
Contributor

That being said, I don't think doing something FreeBSD-specific for this is appropriate: if a system doesn't have gcc, configure should try using clang, regardless of the host OS.

@dhuseby

dhuseby commented Jun 10, 2015

Copy link
Copy Markdown
Author

@wca No, it doesn't. See here: http://buildbot.rust-lang.org/builders/auto-bsd-64-opt/builds/5214/steps/configure/logs/stdio The buildbot has been failing since your commit on June 3rd.

I ran into this because I'm investigating the Bitrig build break caused by the recent __morestack cleanup. So I wanted to build on FreeBSD to see if it is broken there too but the configure step wouldn't finish, so I fixed it 👍

@dhuseby

dhuseby commented Jun 10, 2015

Copy link
Copy Markdown
Author

@wca you're right about trying clang if gcc doesn't exist, but currently the only way to get clang is through forcing it. I suppose after the os-specific blocks, I could add a general test for gcc and enable forcing clang if clang exists.

@wca

wca commented Jun 10, 2015

Copy link
Copy Markdown
Contributor

Yes, that would be better. It follows the spirit of the error log from the buildbot: "either clang or gcc is required", too.

@brson

brson commented Jun 10, 2015

Copy link
Copy Markdown
Contributor

I can't think of any specific reason we've never done that general fallback to clang, but I do worry that if we did we could end up using clang in 'unsupported' configurations, since e.g. people rarely build with clang on Linux.

@alexcrichton Do you have opinions about whether the build should automatically fall back from gcc to clang on all platforms?

@dhuseby

dhuseby commented Jun 10, 2015

Copy link
Copy Markdown
Author

I just changed the PR to be a generic fallback to clang if gcc is unavailable. It seems to work on my FreeBSD box that only has clang.

@wca

wca commented Jun 10, 2015

Copy link
Copy Markdown
Contributor

Thanks @dhuseby!

@brson I can understand a motivation to reduce exposure to problems, but generally speaking it's unusual for any build to require explicit specification to use apparently working compilers, even if they aren't the "usual" one for the current OS.

@dhuseby

dhuseby commented Jun 10, 2015

Copy link
Copy Markdown
Author

@brson I agree with @wca on this one, I think we should always try to fall back on clang if the user hasn't already specified clang and gcc doesn't exist. I don't think it's going to cause too many problems.

@dhuseby

dhuseby commented Jun 10, 2015

Copy link
Copy Markdown
Author

@wca BTW, the __morestack cleanup broke the FreeBSD build too. I'll fix it with OpenBSD and Bitrig.

@brson

brson commented Jun 10, 2015

Copy link
Copy Markdown
Contributor

@bors r+

Well after this patch I've read through a bunch of the build system code for detecting and invoking C compilers and I'm totally mortified. What a mess :(

I can't tell if this change actually changes all invocations of the C compiler or not. It looks to me like it only changes how we build LLVM and on most platforms we just invoke cc unconditionally for all other C builds.

@bors

bors commented Jun 10, 2015

Copy link
Copy Markdown
Collaborator

📌 Commit e3747d1 has been approved by brson

@wca

wca commented Jun 10, 2015

Copy link
Copy Markdown
Contributor

@brson Unfortunately, that kind of issue isn't all that uncommon. I submitted a pull request a few weeks ago for a servo crate build that had 'gcc' hardcoded as the C compiler command. I would think Cargo would handle & offer a library for invoking C and C++ compilers for crates. The usual environment variables (CC, CXX, CFLAGS, CXXFLAGS, LDFLAGS, and so forth) are a good mechanism for build-time configuration when an user wants something other than the auto-detected defaults.

@bors

bors commented Jun 11, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit e3747d1 with merge e32cce5...

@bors

bors commented Jun 11, 2015

Copy link
Copy Markdown
Collaborator

💔 Test failed - auto-linux-32-opt

@wca

wca commented Jun 11, 2015

Copy link
Copy Markdown
Contributor

What kind of gcc doesn't like --version?

@dhuseby

dhuseby commented Jun 11, 2015

Copy link
Copy Markdown
Author

hrm...let me see if I can devise a better test for gcc.

@brson

brson commented Jun 24, 2015

Copy link
Copy Markdown
Contributor

@bors: retry

@brson

brson commented Jun 24, 2015

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Jun 24, 2015

Copy link
Copy Markdown
Collaborator

📌 Commit 44d487d has been approved by brson

@bors

bors commented Jun 24, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 44d487d with merge 4757d9b...

bors added a commit that referenced this pull request Jun 24, 2015
On FreeBSD machines without GCC installed, the configure script will now fall back to using clang.
@bors

bors commented Jun 24, 2015

Copy link
Copy Markdown
Collaborator

@bors bors merged commit 44d487d into rust-lang:master Jun 24, 2015
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 this pull request may close these issues.

5 participants