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

Python 2.7.12 #62

Merged
merged 14 commits into from
Aug 12, 2016
Merged

Python 2.7.12 #62

merged 14 commits into from
Aug 12, 2016

Conversation

jjhelmus
Copy link
Contributor

@jjhelmus jjhelmus commented Jul 27, 2016

closes #2

@jjhelmus
Copy link
Contributor Author

Rebase of PR #46 against branch 2.7.

This should be ready for review. There were a few patches which are applied in anaconda-recipe mentioned in the older PR which might still need some discussion.

@jjhelmus
Copy link
Contributor Author

I've compared the contents of a win-64 package created with this recipe against the contents of the defaults package and the list of files is nearly identical. The main differences are:

  • defaults includes python2712.chm in the Docs directory, this does not. I do not think anyone uses this file anymore.
  • README files in the Tools directory have a ".TXT" ending in the defaults package vs no ending. This recipe also include the pynche and dutree.doc files in this directory.
  • The info directories are different as the recipe is included in this package.

I do not believe any of these difference show issue with either package, just different choices in what and how various files are included.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@jjhelmus
Copy link
Contributor Author

Good to see Travis CI and the conda-forge-linter decided to join the party.

@jjhelmus
Copy link
Contributor Author

The only questions I have before merging this is if the following patches from anaconda-recipes should be included. From my review I do not think they are needed but I would appreciate another set of eyes.

  • linux-clear_history.patch : Does some modification to readline, perhaps to address Extra space with tab completion in ipython ContinuumIO/anaconda-issues#42. I do not think we need this patch, I check in a VM and no extra space is being added to tab completions in IPython or Python with readline and rlcompleter.

  • fix-readline-extra-space.patch : Performs some modification to setup.py to force system paths to the end of lists of directories. I do not think this is needed as the post-build step takes care of correcting any linkages to system libraries.

  • osx-site.patch : Something to do with /python.app which is not included in this build.

  • win-cygwin.patch : Modifies distutils to better support cygwin? Personally I think we should ship distutils as-is without modification.

    The other patches mentioned in WIP: Python 2.7.12 #46 did not seem to be relevant to non-Continuum builds

@jjhelmus jjhelmus mentioned this pull request Jul 27, 2016
@jakirkham
Copy link
Member

Generally agree with your thoughts on these 4 patches above. Brief thoughts included below.

linux-clear_history.patch fix-readline-extra-space.patch

Happy to punt on the readline stuff. If it doesn't work, it will be mildly annoying to people (probably will be really annoying to me if it doesn't work meaning it will get fixed quickly 😄). Though it won't be debilitating. Probably our other changes have mostly eliminated the need for them.

osx-site.patch

Once we figure out how we want to do Python GUI stuff, we can reassess whether we need it.

win-cygwin.patch

Basically agree with you. Though maybe some more savvy Windows people should weigh in.

@pelson
Copy link
Member

pelson commented Aug 8, 2016

In principle I have no reason to object on this change. I haven't looked at the detail, but I'm aware that we have packaged version 1.0-2.0 and 3.4-3.6.dev yet not packaged 2.7 and would love for us to get the only priority legacy version of Python 😉 .

@jjhelmus
Copy link
Contributor Author

jjhelmus commented Aug 9, 2016

Can anyone with Windows experience have a look at this PR? @msarahan?

@jakirkham
Copy link
Member

Also it would be great if @JanSchulz and @patricksnape had a chance to take a look at it.

) else (
set PLATFORM=Win32
set VC_PATH=x86
set BUILD_PATH=win32
set PCB=%SRC_DIR%\PCbuild
call "C:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\bin\vcvars32.bat"
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary? Shouldn't conda-build be taking care of this? Also, by defining these, I think that you are precluding use of the Visual C++ compiler for Python tools, which is really the only way that most people have access to these compilers, especially on 64-bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I had issues building Python itself with the VC++ compiler for Python tools and so I added in the explicit call. I'll try removing it to see what happens at this might have just been an issue on my testing machine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Must have been something on my machine as this works fine on AppVeyor.

@msarahan
Copy link
Member

msarahan commented Aug 9, 2016

Looks fine to me (without taking the time to actually build it - but I'm assuming Appveyor did OK here. I had just a few small questions on the recipe itself.

Setting up the necessary variables for Visual C++ is taken care of by
conda-build.  Remove the explicit call to vcvars*.bat from the recipe.
@jjhelmus
Copy link
Contributor Author

Added a jinja templated for the version and sha256 and removed unneeded VC++ setup from Windows build script. Should be good to go now.

@msarahan msarahan merged commit caeee50 into conda-forge:2.7 Aug 12, 2016
@msarahan
Copy link
Member

Thanks @jjhelmus

@minrk minrk mentioned this pull request Sep 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants