Changed +fpic to +pic variant

[citibeth]

This is a decent start. Packages need to be checked individually to conform to #2375

[citibeth]

@aprokop Are you able to fix up any of these packages?

[aprokop]

@citibeth If by fixing you mean a simple replacing with "{0}".format(self.compiler.pic_flag), sure, I could do that. But I won't have time to test all (or maybe even any) of them at this point.

[citibeth]

yes thanks that would be great

…

On Dec 5, 2016 10:38 AM, "Andrey Prokopenko" ***@***.***> wrote: @citibeth <https://github.com/citibeth> If by fixing you mean a simple replacing with "{0}".format(self.compiler.pic_flag), sure, I could do that. But I won't have time to test all (or maybe even any) of them at this point. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2479 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd6IC5DMCIW-FA6Wh3-RRjKpV3hRYks5rFC_sgaJpZM4LDrGF> .

[adamjstewart]

I think it would be good to convert +fpic to +pic and convert -fPIC to self.compiler.pic_flag in the same PR, as they touch a lot of the same packages. @citibeth If it isn't set already, can you allow @aprokop to push directly to this PR?

[citibeth]

@aprokop has been issued an invitation to my fork.

…

On Mon, Dec 5, 2016 at 11:09 AM, Adam J. Stewart ***@***.***> wrote: I think it would be good to convert +fpic to +pic and convert -fPIC to self.compiler.pic_flag in the same PR, as they touch a lot of the same packages. @citibeth <https://github.com/citibeth> If it isn't set already, can you allow @aprokop <https://github.com/aprokop> to push directly to this PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2479 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd4CKWW1fAeQa6uCgYoeQxC0Ync-Hks5rFDcegaJpZM4LDrGF> .

[aprokop]

Before my push, I did check for flake8, and fixed those except long lines. What's the policy on it?

[becker33]

@aprokop Any commit will fail the CI tests if it does not satisfy flake8.

[aprokop]

@becker33 Then, is the flake8 feature was added just recently? 'Cause the failures are stemming from long lines already present in the repo.

Edited:
Actually, let me rephrase it. Some packages.py do not satisfy flake8, but it seems that CI fails only on newly added lines. The lines that were already failing flake8 are ignored.

[adamjstewart]

@aprokop I'm also confused as to why it is complaining about line 70 of superlu-dist, as that line isn't too long. The flake8 logic has been in place for a long time, and there should be no packages in Spack that fail. Btw, we do have some exceptions to the regular flake8 tests. They can be seen in $SPACK_ROOT/.flake8 and spack edit -c flake8.

[aprokop]

@adamjstewart The way I tested locally was with ViM's nvie/vim-flake8 plugin. It did seem to highlight everything beyond 79 symbols in a line. For instance, it highlighted line 34 in scotch:

    url = "http://gforge.inria.fr/frs/download.php/latestfile/298/scotch_6.0.3.tar.gz"

with an error

var/spack/repos/builtin/packages/scotch/package.py|34 col 80| E501 line too long (86 > 79 characters)

That line is already in the repo, so I am confused. Are there different versions of flake 8 (I'm not very familiar with it)?

[becker33]

@aprokop We have flake8 configured to ignore any url lines, for example. @adamjstewart pointed you towards those changes correctly.

[aprokop]

@becker33 Thanks! I should start reading comments more carefully. I'll update the PR to fix the issues introduced by me.

[adamjstewart]

Here are the specific lines that ignore things like long URLs and long version() directives:
https://github.com/LLNL/spack/blob/develop/lib/spack/spack/cmd/flake8.py#L49

If you want to test locally for flake8 compliance, simply use:

$ spack flake8

More info can be found in the documentation:
http://spack.readthedocs.io/en/latest/contribution_guide.html#flake8-tests

[aprokop]

@adamjstewart It's funny, but running spack flake8 on this branch produces no errors for me. On the other hand, running share/spack/qa/run-flake8-tests produces

../../../var/spack/repos/builtin/packages/scotch/package.py:98: [E501] line too long (81 > 79 characters)
../../../var/spack/repos/builtin/packages/superlu-dist/package.py:68: [E501] line too long (103 > 79 characters)
../../../var/spack/repos/builtin/packages/yaml-cpp/package.py:32: [E901] SyntaxError: unexpected character after line continuation character

which is similar to the failing CI except for additional presence of yaml-cpp.

[adamjstewart]

That's very interesting, because run-flake8-tests actually calls spack flake8. I think the problem is that the branch isn't up-to-date. We had a bug in spack flake8 soon after we switched that was fixed months ago. Can you try rebasing on the latest develop?

[aprokop]

Rebasing the branch on top of develop (9edb31a) does not fix it:

3a15778f103e spack (test) $ spack flake8
=======================================================
flake8: running flake8 code checks on spack.

Modified files:
=======================================================

Flake8 checks were clean.

3a15778f103e spack (test) $ ./share/spack/qa/run-flake8-tests 
Dependencies found.
=======================================================
flake8: running flake8 code checks on spack.

Modified files:
  var/spack/repos/builtin/packages/adios/package.py
  var/spack/repos/builtin/packages/cantera/package.py
  var/spack/repos/builtin/packages/hdf/package.py
  var/spack/repos/builtin/packages/metis/package.py
  var/spack/repos/builtin/packages/mumps/package.py
  var/spack/repos/builtin/packages/netcdf/package.py
  var/spack/repos/builtin/packages/netlib-scalapack/package.py
  var/spack/repos/builtin/packages/openblas/package.py
  var/spack/repos/builtin/packages/otf2/package.py
  var/spack/repos/builtin/packages/parallel-netcdf/package.py
  var/spack/repos/builtin/packages/parmgridgen/package.py
  var/spack/repos/builtin/packages/scorep/package.py
  var/spack/repos/builtin/packages/scotch/package.py
  var/spack/repos/builtin/packages/suite-sparse/package.py
  var/spack/repos/builtin/packages/sundials/package.py
  var/spack/repos/builtin/packages/superlu-dist/package.py
  var/spack/repos/builtin/packages/superlu/package.py
  var/spack/repos/builtin/packages/trilinos/package.py
  var/spack/repos/builtin/packages/yaml-cpp/package.py
  var/spack/repos/builtin/packages/zoltan/package.py
=======================================================
var/spack/repos/builtin/packages/scotch/package.py:98: [E501] line too long (81 > 79 characters)
var/spack/repos/builtin/packages/superlu-dist/package.py:68: [E501] line too long (103 > 79 characters)
var/spack/repos/builtin/packages/yaml-cpp/package.py:32: [E901] SyntaxError: unexpected character after line continuation character

Flake8 found errors.

[aprokop]

@adamjstewart Nevermind, my PATH was messed up, and spack was picked up from an older repo. With a fixed path spack flake8 produces the same output as running the script.

[aprokop]

What else do we need for this PR? Should we just squash, rebase and push?

[adamjstewart]

Aside from rebasing, I don't see anything this PR is missing. I trust that you haven't missed any packages.

Unfortunately, this PR has the side effect that none of these packages will be able to compile with the NAG compiler anymore. But I think I'm the only one who uses that anyway.

[tgamblin]

@adamjstewart: how does this break NAG?

[citibeth]

Between the NAG issues and the next gen shared flag we have discussed, I am not thrilled with this PR.

…

On Dec 8, 2016 11:49 AM, "Todd Gamblin" ***@***.***> wrote: @adamjstewart <https://github.com/adamjstewart>: how does this break NAG? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2479 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd6w1SxjenR8Wcd89jlku7B7Ocn0Cks5rGDUqgaJpZM4LDrGF> .

[adamjstewart]

@citibeth This is your PR 😆

@tgamblin The problem here is that the pic_flag support that was added in #2375 doesn't support mixed compilers yet. If I build with NAG, my self.compiler.pic_flag is set to -PIC, which is correct for NAG, but my CC/CXX compilers are GCC, which require -fPIC.

[tgamblin]

@adamjstewart: two thoughts. One, we could provide separate c_pic_flag, cxx_pic_flag, f77_pic_flag, fc_pic_flag variables on the compiler class, but that gets cumbersome for the package author. The other idea would be to have the compiler wrappers just insert the right pic flag to remove the burden from the package author. The compiler wrappers know which compiler they are.

[adamjstewart]

Either would be fine by me. It's not an urgent need. The only thing I use NAG for is building HDF4/5 and NetCDF, but neither build with NAG at the moment, so I'm waiting for the next release to come out.

[aprokop]

@adamjstewart wrote:

Aside from rebasing, I don't see anything this PR is missing. I trust that you haven't missed any packages.

There are few places around the place with hardcoded -fPIC in the Makefiles. Not sure what to do with those.

I guess the NAG issue is bothersome. How soon do people expect to properly solve the whole shared/static/pic flags issue?

I wanted to participate in today's call but will have to skip it due to conflicts. Can somebody bring the issue up there, and maybe define steps to be done?

[adamjstewart]

There are few places around the place with hardcoded -fPIC in the Makefiles. Not sure what to do with those.

Meh. I'll fix those as I encounter them. I don't build that many packages with NAG.

[tgamblin]

@aprokop: it would be great to have you on the call. I am not sure we'll get the flags stuff done for 0.10, but we should discuss it on the next call. Will you be able to make next week?

[aprokop]

@tgamblin Not sure yet. I have a recurring meeting at that time, but I'll see if I can skip it.

[citibeth]

Can I close this PR? I don't think it should be merged. I do think we should:

1. For now, use the pic_flag that's been added to compilers in Adding pic_flag property to compilers #2375. Packages should never have the string '-fPIC' in them. @adamjstewart would this solve your problems?

2. Include the PIC stuff in a universal variant called build_type that can take either the values static or shared; see Re-work shared vs. static builds (Universal Variants) #2492. This will be possible once variant: added support for multi-valued variants #2386 is merged (and maybe once we get key technology from All CMakePackages depend on CMake #2466).

[aprokop]

@citibeth So, just merge the patches I did that switch to pic_flag without merging your patch?

[adamjstewart]

For now, use the pic_flag that's been added to compilers in #2375. Packages should never have the string '-fPIC' in them. @adamjstewart would this solve your problems?

Actually, most of the packages work correctly for me at the moment. The problem is that NAG (which only accepts -PIC) does not provide C/C++ compilers, so I mix it with GCC (which only accepts -fPIC). For packages that only specify -fPIC for CC/CXX, switching to pic_flag would actually break things for NAG. For packages that only specify -fPIC for F77/FC, the switch would help. I think the former is more common than the latter.

Basically, this change will make things worse for NAG. But I think the intent is good. The end goal will be to be able to specify CC/CXX pic_flags and F77/FC pic_flags separately. I am not opposed to this change going in, as I'm probably the only one using NAG and it isn't able to build the HDF/NetCDF stack I want at the moment anyway. Although, I'm the one who requested the pic_flag, and I don't think -fPIC is a problem for any non-NAG compiler that we support. Either we should revamp the pic_flags to be specific to each CC/CXX/F77/FC compiler, or we should merge anyway or wait until better mixed compiler/compiler dependency support is added.

[aprokop]

@adamjstewart Out of curiosity, have you tried talking to NAG folks and see if they are open to adding support to -fPIC in the future?

[adamjstewart]

I have not. @ThemosTsikas, are you aware of any plans for NAG to support -fPIC as an alias of -PIC? (maybe it already does for all I know)

[citibeth]

Basically, this change will make things worse for NAG. But I think the intent is good. The end goal will be to be able to specify CC/CXX pic_flags and F77/FC pic_flags separately.

So... does this mean that #2375 does not work with mixed toolchains? If so, I suggest we focus on fixing that issue, and then converting existing packages to use pic_flag --- or whatever pic_flag becomes once #2375 has been extended to work with mixed toolchains.

[ThemosTsikas]

Hello, we have no plans to add -fPIC/-fpic options to the NAG Fortran Compiler.