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

make symlinking python3->python optional #7960

Merged

Conversation

citibeth
Copy link
Member

@citibeth citibeth commented May 1, 2018

@adamjstewart @healther

This PR makes #7103 optional. I believe #7103 is not a good idea for the following reasons:

  1. It is not the way the Python distro does things. python3 is the standard name for the Python3 executable, and python, according to the standard, always means Python2. By default, Spack installations should conform to the standard.

  2. It does weird things to my Spack. Consider what I did:

    1. Download Spack, start using it. (Spack running with my System's /usr/bin/ptyhon).
    2. Install my software stack, which includes Python3
    3. Do a module load of my software stack.
    4. All of a sudden, the spack command is now running with Python3, not Python2. What horrible inconsistencies might that cause, AFTER I've built a lot of stuff? (To be fair, Spack works with Python3 too. But it still makes me nervous).
  3. It breaks some builds. I have builds that assume, as per the standard, that "python" means Python2. (Admittedly, these builds should probably be fixed to use the python2 executable).

Anyway... I think it's best to leave the symlinking as optional. So now there's a python+symlink variant.

@adamjstewart
Copy link
Member

I vote for defaulting to True. That's what Anaconda does.

@adamjstewart
Copy link
Member

From what I understand, the Python standard says to use:

#!/usr/bin/env python3

for Python 3-only code,

#!/usr/bin/env python2

for Python 2-only code, and

#!/usr/bin/env python

for code that is compatible with both. So symlinking python3 -> python doesn't violate this.

@citibeth
Copy link
Member Author

citibeth commented May 1, 2018

Can we just follow PEP 394? https://legacy.python.org/dev/peps/pep-0394/

for the time being, all distributions should ensure that python refers to the same target as python2...
The more general python command should be installed whenever any version of Python 2 is installed and should invoke the same version of Python as the python2 command...

Rationale

This recommendation is needed as, even though the majority of distributions still alias the python command to Python 2, some now alias it to Python 3 ([5]). As some of the former distributions did not provide a python2 command by default, there was previously no way for Python 2 code (or any code that invokes the Python 2 interpreter directly rather than via sys.executable) to reliably run on all Unix-like systems without modification, as the python command would invoke the wrong interpreter version on some systems, and the python2 command would fail completely on others. The recommendations in this PEP provide a very simple mechanism to restore cross-platform support, with minimal additional work required on the part of distribution maintainers.

Future Changes to this Recommendation

It is anticipated that there will eventually come a time where the third party ecosystem surrounding Python 3 is sufficiently mature for this recommendation to be updated to suggest that the python symlink refer to python3 rather than python2.

This recommendation will be periodically reviewed over the next few years, and updated when the core development team judges it appropriate. As a point of reference, regular maintenance releases for the Python 2.7 series will continue until at least 2020.

@citibeth citibeth changed the title Make symlinking python3->python optional Follow PEP 394: make symlinking python3->python optional May 1, 2018
@healther
Copy link
Contributor

healther commented May 2, 2018

The problem with respecting PEP 394 is that it breaks stuff in practice. There are packages (I don't remember which from the top of my head) that REQUIRE python pointing to the python installation that you want to use it with. You can look through the PRs/issues I opened in that regard (I'm uncertain when I can find the time to do it myself)

I know at least of ArchLinux that does the python->python3 link on system level (yes, yes we can discuss about the sensibility of that, but that's besides the point).

All of a sudden, the spack command is now running with Python3, not Python2. What horrible inconsistencies might that cause, AFTER I've built a lot of stuff? (To be fair, Spack works with Python3 too. But it still makes me nervous).

If spack has inconsistencies between python2 and python3 implementations I'd consider this a bug in spack! As @adamjstewart said if you use #!/usr/bin/env python your code better doesn't depend on a specific python version.

I'm not opposed to making it optional, but the default should be True. We should at least make it a conscious decision for people to stick with future-incompatible code.
And I'm especially worried about system dependencies creeping in again. We went through a lot of pain when we moved from a standard ubuntu to a minimal distribution, with the idea of relying on spack rather than aptitude. In that move we found a lot of missing dependencies in spack. I really don't want to go back to "relying on the system being in some defined state"

@citibeth
Copy link
Member Author

citibeth commented May 2, 2018

@tgamblin @scheibelp

All of a sudden, the spack command is now running with Python3, not Python2. What horrible inconsistencies might that cause, AFTER I've built a lot of stuff? (To be fair, Spack works with Python3 too. But it still makes me nervous).

If spack has inconsistencies between python2 and python3 implementations I'd consider this a bug in spack! As @adamjstewart said if you use #!/usr/bin/env python your code better doesn't depend on a specific python version.

This would be a useful discussion to have; EXCEPT that it has already been had for us, and an answer decided upon by the wider Python community in PEP 394. At this point, the issue isn't what we think is right or wrong, or what we personally want, or what Anaconda or ArchLinux did; it's simply a matter of reading and following the standard.

The problem with respecting PEP 394 is that it breaks stuff in practice.

Unfortunately, as the PEP 394 authors point out, NOT following PEP 394 breaks other stuff in practice. In fact, it broke things for me, which is how I noticed this in the first place. In cases like this, standards serve to determine where the bug lies, and therefore where more effort is required to make things work. PEP 394 makes it clear that a package that any package that REQUIRES python if it wants Python3 IS buggy; but that anything that builds with python->python2 but fails with python->python3 is actually NOT buggy. At least until 2020, after which they will revisit this issue.

Non-standard implementations of the platform only serve to perpetuate buggy (i.e. non-standard) packages that rely on it. We've seen similar stuff in C++ compilers for years. How many times has GNU removed a non-standard feature from their compiler, breaking existing non-standard code --- while adding a command-line switch for people who ABSOLUTELY HAVE to have the non-standard feature or behavior. As systems move toward increasing standards compliance, some people grumble, but eventually everything works better together.

I do not dispute that some packages currently require non-standard Pythons. Moving the entire python package away from standards compliance, and breaking other standards-compliant packages, is NOT the answer. If we had more specifics on what these packages are, we could help fix them; here are some possible general approaches:

  1. Use depends_on('python+symlink') in them. Easy. Of course, this will make them incompatible with packages that depends_on('python~symlink'), so it's not ideal.
  2. Maybe there's a way to tell them explicitly the name of the Python executable. This is a possibility with many packages I've seen.
  3. In the Spack build, create a local python symlink specifically for the non-standard package that requires it, in a place where only that one package will find it.
  4. Add a patch to the Spack package.
  5. Fix the upstream distro.

And I'm especially worried about system dependencies creeping in again.

Reliance on non-standard behavior is BY DEFINITION a system dependency.

We went through a lot of pain when we moved from a standard ubuntu to a minimal distribution

Had you stuck with Ubuntu, you would also have this issue: Python3 is now the default and the python command has been removed (unless you've installed Python2):

https://wiki.ubuntu.com/Python/3

@citibeth
Copy link
Member Author

citibeth commented May 2, 2018

@tgamblin @scheibelp I need this merged because #7103 breaks some builds. We are all in agreement on the PR, other than whether it should default to +symlink or ~symlink. Can you please make a final decision and can get this merged soon?

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 think this is fine but I think the variant should be called pythoncmd or something a bit more descriptive than “symlink”.

I would vote with @healther and @adamjstewart to default True for now and to try to make packages in Spack follow this part of PEP 394:

python should be used in the shebang line only for scripts that are source compatible with both Python 2 and 3

I realize it also says that distros should ensure that python is python2, but I think Spack can afford to be a bit more future looking than the distros, as we have options like this variant.

I’m fine to merge with the changes above.

I like the suggestion for the variant name.  I didn't like `symlink` but couldn't think of anything better.
@citibeth citibeth added the ready label May 2, 2018
@healther
Copy link
Contributor

healther commented May 2, 2018

Maybe there's a way to tell them explicitly the name of the Python executable. This is a possibility with many packages I've seen.

There was at least one package which didn't allow for this. That was the whole reason for adding the symlinking in the first place.

I'd really say we should be future looking and not backwards. Adhering to a spec is not better per se. The argument should ALWAYS be "it makes sense" not "it was written ".

This will come back an bite us. spack is supposed to serve the scientific community. Stuff that is NOW run and used and developed WILL be used far beyond 2020. I can override this for us, so it's not a showstopper for me... But really why assume python->python2 at all (which will be changed in the future, you even quoted that from the PEP)

@citibeth
Copy link
Member Author

citibeth commented May 2, 2018

@healther Did you see recent communication on this thread? Please review and speak up if you would like to see any further changes; otherwise, this PR is ready to merge.

@healther
Copy link
Contributor

healther commented May 2, 2018

Yes I saw it, I'd prefer the default to be +pythoncmd but if I'm the sole one then merge it as is. My prediction is: It will end up coming back annoying us later if we default to False.

btw: I restarted Travis as the coverage stuff failed.

@citibeth
Copy link
Member Author

citibeth commented May 2, 2018 via email

@tgamblin
Copy link
Member

tgamblin commented May 2, 2018

@healther: we went with default +pythoncmd, and @citibeth updated the PR above to reflect that.

@adamjstewart
Copy link
Member

btw: I restarted Travis as the coverage stuff failed.

@healther see the note at the bottom of http://spack.readthedocs.io/en/latest/contribution_guide.html#coverage

@tgamblin tgamblin changed the title Follow PEP 394: make symlinking python3->python optional make symlinking python3->python optional May 2, 2018
@tgamblin tgamblin merged commit cbd77e3 into spack:develop May 2, 2018
@adamjstewart adamjstewart deleted the efischer/180501-OptionalPython3Symlink branch May 2, 2018 19:26
@healther
Copy link
Contributor

healther commented May 2, 2018

@adamjstewart I know but it's also a step in Travis, if that fails with a non-zero error code the next stage is not triggered. I should have been more precise to say that the coverage report generation failed

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

Successfully merging this pull request may close these issues.

4 participants