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

Inherit CFLAGS & LDFLAGS from outside environment. #245

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@lamby
Contributor

lamby commented Dec 15, 2017

In Debian we use the dpkg-buildflags mechanism to control
enabling/disabling of hardening-related buildflags such as relro, PIE
etc.

However, the current Makefile currently ignores those for CFLAGS and
then ignored them when linking.

Inherit CLFLAGS & LDFLAGS from outside environment.
In Debian we use the dpkg-buildflags mechanism to control
enabling/disabling of hardening-related buildflags such as relro, PIE
etc.

However, the current Makefile currently ignores those for CFLAGS and
then ignored them when linking.

@lamby lamby changed the title from Inherit CLFLAGS & LDFLAGS from outside environment. to Inherit CFLAGS & LDFLAGS from outside environment. Dec 15, 2017

@lamby

This comment has been minimized.

Show comment
Hide comment
@lamby

lamby Dec 15, 2017

Contributor

CLFLAGS? CFLAGS.

Contributor

lamby commented Dec 15, 2017

CLFLAGS? CFLAGS.

@mnunberg

This comment has been minimized.

Show comment
Hide comment
@mnunberg

mnunberg Dec 15, 2017

Collaborator

Seems this fails because $MODULE_ARTIFACT isn't set to anything. Otherwise CI looks good.

Collaborator

mnunberg commented Dec 15, 2017

Seems this fails because $MODULE_ARTIFACT isn't set to anything. Otherwise CI looks good.

@lamby

This comment has been minimized.

Show comment
Hide comment
@lamby

lamby Dec 15, 2017

Contributor

Seems this fails because $MODULE_ARTIFACT isn't set to anything

Mmm, that seems unrelated to this PR, so shouldn't block merge AFACT?

Contributor

lamby commented Dec 15, 2017

Seems this fails because $MODULE_ARTIFACT isn't set to anything

Mmm, that seems unrelated to this PR, so shouldn't block merge AFACT?

@dvirsky

This comment has been minimized.

Show comment
Hide comment
@dvirsky

dvirsky Dec 19, 2017

Contributor

@mnunberg the problem is that we're hiding some environment variables from PRs, this is a security measure (otherwise PRs can add a line to print our S3 credentials which are exposed as env vars to builds). This means PRs never build successfully. I'll see what I can do to mitigate this.

Contributor

dvirsky commented Dec 19, 2017

@mnunberg the problem is that we're hiding some environment variables from PRs, this is a security measure (otherwise PRs can add a line to print our S3 credentials which are exposed as env vars to builds). This means PRs never build successfully. I'll see what I can do to mitigate this.

@dvirsky

This comment has been minimized.

Show comment
Hide comment
@dvirsky

dvirsky Dec 19, 2017

Contributor

@lamby for some reason github's useless conflict editor can't resolve the stupid conflict here, so I'm just applying this manually onto master. However I'm not changing $LD to $CC since it breaks the build on Mac Os.

Contributor

dvirsky commented Dec 19, 2017

@lamby for some reason github's useless conflict editor can't resolve the stupid conflict here, so I'm just applying this manually onto master. However I'm not changing $LD to $CC since it breaks the build on Mac Os.

@dvirsky dvirsky closed this Dec 19, 2017

dvirsky added a commit that referenced this pull request Dec 19, 2017

@lamby

This comment has been minimized.

Show comment
Hide comment
@lamby

lamby Dec 19, 2017

Contributor

I'm not changing $LD to $CC since it breaks the build on Mac Os.

Right, but then you can't use the linker pass-through variables that dpkg-buildflags exports, rendering the change somewhat useless. :(

Contributor

lamby commented Dec 19, 2017

I'm not changing $LD to $CC since it breaks the build on Mac Os.

Right, but then you can't use the linker pass-through variables that dpkg-buildflags exports, rendering the change somewhat useless. :(

@dvirsky

This comment has been minimized.

Show comment
Hide comment
@dvirsky

dvirsky Dec 19, 2017

Contributor

you want to create an OS based variable for the linker? I'll live with that.

Contributor

dvirsky commented Dec 19, 2017

you want to create an OS based variable for the linker? I'll live with that.

@lamby

This comment has been minimized.

Show comment
Hide comment
@lamby

lamby Dec 19, 2017

Contributor

you want to create an OS based variable for the linker? I'll live with that.

Don't quite follow that, sorry :)

Contributor

lamby commented Dec 19, 2017

you want to create an OS based variable for the linker? I'll live with that.

Don't quite follow that, sorry :)

@dvirsky

This comment has been minimized.

Show comment
Hide comment
@dvirsky

dvirsky Dec 19, 2017

Contributor
LINKER=$LD
ifneq ($(uname_S),Darwin)
  LINKER=$CC
endif
Contributor

dvirsky commented Dec 19, 2017

LINKER=$LD
ifneq ($(uname_S),Darwin)
  LINKER=$CC
endif

dvirsky added a commit that referenced this pull request Dec 19, 2017

@dvirsky

This comment has been minimized.

Show comment
Hide comment
@dvirsky

dvirsky Dec 19, 2017

Contributor

@lamby try it now and see if it works fine

Contributor

dvirsky commented Dec 19, 2017

@lamby try it now and see if it works fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment