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

Language-specific PIC flags #15474

Merged
merged 4 commits into from
Apr 17, 2020
Merged

Conversation

adamjstewart
Copy link
Member

#2375 added compiler-specific PIC flags.

This PR adds language-specific PIC flags.

Fixes #15445

@aradi can you test this?

@aradi
Copy link

aradi commented Mar 12, 2020

The patch solves the compiler dependent pic flag problem. However, compilation still fails since zlib is linked with the C-compiler (gcc), but gets flags passed (-Wl,-Wl,,) which are meant for the cases, where the NAG Fortran compiler is used for the linking. So, maybe we would need a similar language dependent separation also for the linker flag?

@adamjstewart
Copy link
Member Author

@aradi good point, let me open up a separate PR to address that.

@adamjstewart
Copy link
Member Author

@alalazo it looks like you added the linker_arg logic in #9168. When does this arg get used? Maybe we don't even need a special linker_arg for NAG if it's only used by the linker, not the compiler.

@alalazo
Copy link
Member

alalazo commented Mar 13, 2020

Not sure this is the right direction to go. Do I understand correctly that this works just because nag overrides only the Fortran compilers and uses the defaults for C and C++? What if I use a C compiler that doesn't support -fPIC, would that work?

@adamjstewart
Copy link
Member Author

You are correct, that’s how we’ve handled all language-specific flags in the past. We do this for NAG and Clang (assume GCC is the other half of the compiler).

@adamjstewart
Copy link
Member Author

From what I can tell, #15443 is a step in the right direction, but better mixed compiler support is still a long way off. Without this PR, NAG users can't build anything right now, due to both the self.compiler.pic_flag and self.compiler.linker_arg usage.

@adamjstewart
Copy link
Member Author

@alalazo it looks like you added the linker_arg logic in #9168. When does this arg get used? Maybe we don't even need a special linker_arg for NAG if it's only used by the linker, not the compiler.

Ping @alalazo

@adamjstewart
Copy link
Member Author

Ping ping @alalazo

@alalazo alalazo added this to In progress in Spack v0.15.0 release via automation Mar 27, 2020
@adamjstewart
Copy link
Member Author

Ping ping ping @alalazo

@adamjstewart
Copy link
Member Author

Ping x4 @alalazo @tgamblin

@alalazo
Copy link
Member

alalazo commented Apr 14, 2020

@adamjstewart I put this in the 0.15.0 dashboard (and not 0.14.X) since I think it needs some discussions on the opportunity to merge a workaround that is fairly large and we know it needs to be undone in the mid-long term (because of the reworking of compilers).

If it was a 10 lines change I would merge it straight away, likewise if it was a more permanent fix without its own issues. In this case I think more discussion is needed as we are dealing with something sizeable and that comes with known limitations.

@adamjstewart
Copy link
Member Author

I agree that more discussion is welcome (thanks for pinging more reviewers). The long and short of it is that it is currently impossible to use the NAG compiler with Spack because:

  1. Any package that uses self.compiler.pic_flag for C/C++ code is broken, and
  2. Any package that uses self.compiler.linker_arg (which I think is all packages??) is broken

I would hate to see the NAG compiler go unsupported for months while we wait for 0.15.0, although I'm fine with reaching a consensus on this PR and merging it into develop in the meantime. Also see #15445 and #15776 for more discussion.

Adding @ThemosTsikas to the discussion as our official NAG representative.

Copy link
Member

@tgamblin tgamblin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this change -- it's consistent with how we currently handle different which, as Massimiliano points out, is less than optimal. I also agree that we should enable NAG to continue functioning until we get to 0.15, where we'll have compilers as dependencies.

So, I think this is the right solution for now, and @adamjstewart has already done it.

I would say that maybe we should think about avoiding 4 prefixes for every attribute on compiler, and enabling something like self.cc.pic_flag, self.cxx.pic_flag, etc. on packages (it would reduce redundant code). But since we're redoing this anyway and things like c and cxx will eventually be virtuals, I don't think it's worth the work to do that. So I think this is the best case scenario for now and could be a patch for 0.14.3.

Spack v0.15.0 release automation moved this from In progress to In review Apr 14, 2020
@tgamblin
Copy link
Member

@alalazo: if you're ok with the above, then I am fine with this. Do you have other objections?

@adamjstewart
Copy link
Member Author

I think the only thing left we need to decide (either in this PR or a separate PR) is what to do about the linker_arg issue:

@alalazo it looks like you added the linker_arg logic in #9168. When does this arg get used? Maybe we don't even need a special linker_arg for NAG if it's only used by the linker, not the compiler.

@alalazo
Copy link
Member

alalazo commented Apr 14, 2020

@adamjstewart I'll have a look at linker_arg tomorrow and post here.

@adamjstewart
Copy link
Member Author

Since this affects a lot of packages, and users may submit PRs that use the removed self.compiler.pic_flag in the meantime, I'm going to merge this. @alalazo let me know if you figure out what to do about self.compiler.linker_arg and one of us can address that in a follow-up PR. Then we'll have full NAG support again.

@adamjstewart adamjstewart merged commit 284e450 into spack:develop Apr 17, 2020
Spack v0.15.0 release automation moved this from In review to Done Apr 17, 2020
@adamjstewart adamjstewart deleted the features/pic_flag branch April 17, 2020 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

NAG/GNU mixed toolchain problem
4 participants