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

CI: use lld if present (Linux only) #8641

Merged
merged 1 commit into from Jan 2, 2020
Merged

CI: use lld if present (Linux only) #8641

merged 1 commit into from Jan 2, 2020

Conversation

@bcardiff
Copy link
Member

bcardiff commented Jan 2, 2020

EXPORT_CC value can be overridden to disable lld lookup.

@bcardiff bcardiff added this to the 0.33.0 milestone Jan 2, 2020
@bcardiff bcardiff requested a review from RX14 Jan 2, 2020
ifeq ($(shell uname -s),Linux)
EXPORT_CC ?= $(if $(shell command -v ld.lld),CC="cc -fuse-ld=lld")
endif

This comment has been minimized.

Copy link
@RX14

RX14 Jan 2, 2020

Member

How about just

ifeq ($(shell command -v ld.lld >/dev/null && uname -s),Linux)
    CC ?= cc -fuse-ld=lld
endif

then leaving off all the stuff down the page.

This is cleaner, and still lets people override CC in the environment if they wish.

This comment has been minimized.

Copy link
@bcardiff

bcardiff Jan 2, 2020

Author Member

I updated the condition but the CC and other vars are not exported automatically

That is also a reason why an EXPORT variable is used somewhere else in the Makefile.

This comment has been minimized.

Copy link
@straight-shoota

straight-shoota Jan 2, 2020

Member

@RX14 Sounds reasonable. But when overriding CC, you'd need to remember to add -fuse-ld=lld explicitly...

This comment has been minimized.

Copy link
@RX14

RX14 Jan 2, 2020

Member

Writing export CC works, and is much cleaner.

EXPORT_CC value can be overridden to disable lld lookup.
@bcardiff bcardiff force-pushed the ci/use-lld branch from 7c81e99 to 7c6170d Jan 2, 2020
@bcardiff bcardiff merged commit f3924c9 into master Jan 2, 2020
21 checks passed
21 checks passed
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: dist_artifacts Your tests passed on CircleCI!
Details
ci/circleci: dist_darwin Your tests passed on CircleCI!
Details
ci/circleci: dist_docker Your tests passed on CircleCI!
Details
ci/circleci: dist_docker32 Your tests passed on CircleCI!
Details
ci/circleci: dist_docs Your tests passed on CircleCI!
Details
ci/circleci: dist_linux Your tests passed on CircleCI!
Details
ci/circleci: dist_linux32 Your tests passed on CircleCI!
Details
ci/circleci: dist_snap Your tests passed on CircleCI!
Details
ci/circleci: prepare_common Your tests passed on CircleCI!
Details
ci/circleci: prepare_maintenance Your tests passed on CircleCI!
Details
ci/circleci: publish_docker Your tests passed on CircleCI!
Details
ci/circleci: publish_snap Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_dist_linux32_std_on_docker Your tests passed on CircleCI!
Details
ci/circleci: test_dist_linux_on_docker Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32_std Your tests passed on CircleCI!
Details
ci/circleci: test_preview_mt Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@bcardiff bcardiff deleted the ci/use-lld branch Jan 2, 2020
@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Jan 2, 2020

Please don't merge things before the person who reviewed has had a chance to respond to review followups! Especially when the fix merged wasn't the same as the one suggested.

@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Jan 2, 2020

I agree. I've approved the PR because it seemed fine to me, but I also expected we'd wait for your approval before merging. Maybe there's a communication error and @bcardiff assumed I was sending my approval to merge this immediately.
This has been broken for quite some time so it shouldn't matter whether we merge it a few hours sooner or later.

In general, I'd prefer to wait at least 24 hours before merging a PR to giver others a chance to review and comment. I'm not sure this should go into a policy or something, but it could help to think about that. While it's not a huge issue to apply changes after the PR has been merged via a second PR, it can still cause quite some friction which can be avoided by simply postponing a merge for a few hours. 😉

@bcardiff

This comment has been minimized.

Copy link
Member Author

bcardiff commented Jan 3, 2020

I couldn't make it work with

  CC ?= cc -fuse-ld=lld
  export CC

Adding a @env | grep CC || true to check which the value supposed to be used shows that the default CC=cc is used.

It seems that overriding CC can't be done with ?=.

It also leads to less explicit output. In the current PR the CC="cc -fuse-ld=lld" is displayed explicitly.

Since this is somehow a workaround that might affect some, and using lld is optional I think is better to have an explicit output to allow diagnostic whether lld is used or not.

This trial and error + the approval encourage me to merge it.

Probably I am missing something in my makefile-fu-ness. But I couldn't improve the current solution.

@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Jan 3, 2020

Looks like make has a "prelude" with CC ?= cc in it. So I think the best way to solve this is just to use CC += -fuse-ld=lld and export CC.

I'm not sure about the benefits of having explicit, I personally find it messy. Either way, I appreciate the chance to test my makefile knowledge :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.