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

WIP: Add python-toolchain #642

Closed
wants to merge 3 commits into from

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented May 19, 2016

Partially resolves: #528

This is a "toolchain" for configuring our Python installs. This PR includes a few pieces that still need a bit more polishing.

As a note, this package is designed intentionally to not overlap with the toolchain package. The reason is the toolchain package implies compilation happens, which is an important piece of information to make clear and distinct. By keeping the toolchain package for compiling, it makes it easy for someone to know if compilation is done in a recipe. We do not want to mix this up with Python packages and lose this very important piece of information. This package provided here is only useful for Python builds and does not configure compilers. Though the two could be combined together when both are needed.

The first piece added here are default setuptools arguments. One only needs to run python setup.py install and add python-toolchain as a build dependency to make this work. The only subtle point here is the configuration file we add seems to break distutils. However, I don't actually see this as a problem. We simply don't use this toolchain with distutils. Also, we can find out pretty quickly whether a package uses setuptools or not by doing this experiment. This saves us from hunting around in the code for this information. As we are going to disable the inclusion of pip, setuptools, and wheel as python dependencies on the VMs, it will be much easier to tell if something is building with distutils or not. Basically, people try not using python-toolchain (i.e. using distutils). When it fails, it will need the python-toolchain added too. The rest happens behind the scenes. We have simplified things for that use case by actually making setuptools a run dependency of the python-toolchain to ensure it gets pulled into so only the python-toolchain will be needed as a dependency.

The second piece basically this implements the same idea that @minrk described in this comment. This works by adding a configuration file to $PREFIX/etc/pip.conf or %LIBRARY_PREFIX\etc\pip.conf on Windows. As pip may not know where to find this file and it will not necessarily look in such places we set PIP_CONFIG_FILE in activation scripts to make sure it finds this. In the long run, we will want the functionality asked for in this issue ( conda/conda-build#910 ) to make this work smoothly. In the interim, there is a kludgy one line hack that we can use to get by. In any event, this will allow one to do pip install . and everything will work correctly behind the scenes. Just as mentioned in the previous case, we made things easier by making pip a runtime dependency of the python-toolchain. Should add that regardless of whether the package uses distutils or setuptools, we still find pip install . works fine with the python-toolchain.

This package (just as the toolchain package) was designed so that it could work locally just as well as it does in the CIs. By doing this, we make it easy for people to debug things locally. A feature that remains useful for more complex problems.

Please provide feedback on this. Also, feel free to take it for a test drive. 🚗

cc @msarahan @ocefpaf @pelson @minrk @scopatz @frol

@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 (recipes/python-toolchain) and found it was in an excellent condition.

@jakirkham jakirkham force-pushed the add_python-toolchain branch 2 times, most recently from 253b12c to 4a58f96 Compare May 19, 2016 06:11
@jakirkham
Copy link
Member Author

Running into this issue ( conda/conda-build#963 ). Can come up with a workaround if needed.

@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 (recipes/pint, recipes/python-toolchain, recipes/webcolors) and found it was in an excellent condition.

@jakirkham
Copy link
Member Author

Commented the failing tests for now.

@jakirkham
Copy link
Member Author

Added some recipes for testing. Here pint uses setuptools and webcolors uses distutils.

@jakirkham
Copy link
Member Author

Windows has some issue. I expect this is because I don't know how to activate it correctly. Maybe there are other issues too.

@pelson
Copy link
Member

pelson commented May 19, 2016

Excuse the extremely naive question, but does adopting pip install . --no-deps also solve the problem? I'm not hugely fond of hiding the configuration away from the recipe - I don't think it is a healthy approach to say python setup.py install just works, I can imagine consumers of recipes getting extremely confused.

@jakirkham
Copy link
Member Author

jakirkham commented May 19, 2016

Excuse the extremely naive question, but does adopting pip install . --no-deps also solve the problem?

Unfortunately not. See this comment by @minrk. Basically, to make sure things don't go sideways with pip, we need to add a bunch of options. The good news is that they can be added to a config file, which is one thing we do here. The net result is one can do pip install ..

I'm not hugely fond of hiding the configuration away from the recipe...

This really isn't magical. We are simply adding a 3 line global config file. This is something that setuptools will always use. It lives in $STDLIB_DIR/distutils/distutils.cfg. So, it is not hidden.

...I don't think it is a healthy approach to say python setup.py install just works...

Unfortunately, adding these arguments ( --single-version-externally-managed --record=record.txt ) explicitly has gotten a bit divisive of late. I really don't want our community bikesheding these issues, but I do want to ensure we are able to get the right install behavior simply. One way to do that is configure processes to do this automatically. In this way, it is really no different from the toolchain package. Plus, I want to make sure it is easy to add recipes. The check-for-distutils-or-setuptools-in-the-code dance is getting a little tired. Combined with the fact that we are eliminating setuptools and pip as python dependencies, this becomes really simple. If you need setuptools you add python-toolchain to your build dependencies. If you don't, then you don't add this. If you want to use pip, always add python-toolchain to your build dependencies.

...I can imagine consumers of recipes getting extremely confused.

Well, that's really a question of documentation. I don't see this as any more confusing than the toolchain at this point. Right now, I'm a bit more worried about the fact that Python recipes have no real standardization. This combined with eliminating setuptools and pip should help that.


In any event, I don't want us to get too hung up on this now as I would rather worry about the CI improvements that we are starting with `conda-forge-build-setup` first. That is a really big gain that we are going to need soon as we need to start using `conda-build` 1.20.3 and cleaning out some bottlenecks like the `curl` recipe.

@@ -0,0 +1 @@
set PIP_CONFIG_FILE="%LIBRARY_PREFIX%\etc\pip.conf"
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually it's set "NAME=VALUE". No idea why, but using it that way prevented bugs in cmder

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I have seen that syntax for unsetting a variable, but not setting it. Will change.

@jakirkham jakirkham force-pushed the add_python-toolchain branch from 4aaaddb to 9f74b05 Compare June 2, 2016 21:01
@@ -0,0 +1 @@
set "PIP_CONFIG_FILE="
Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't seen quotes on unset (there are no spaces in this, so it should not be needed), but it will probably also not hurt :-)

@jakirkham
Copy link
Member Author

Can you double check that these activation scripts work correctly on Windows, @JanSchulz? I took my best guess, but I'm still not totally sure that these work right or that I'm activating them correctly.

- test -f "${PREFIX}/etc/pip.conf" # [unix]
#- test -f "${STDLIB_DIR}/distutils/distutils.cfg" # [unix]
- if not exist "%LIBRARY_PREFIX%\\etc\\pip.conf" exit 1 # [win]
#- if not exist "%STDLIB_DIR%\\distutils\\distutils.cfg" exit 1 # [win]
Copy link
Contributor

Choose a reason for hiding this comment

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

In the go recipe I now try

- cmd /c if x%GOROOT% NEQ x%CONDA_DEFAULT_ENV%\go exit 1   # [win]

Copy link
Contributor

Choose a reason for hiding this comment

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

But it seems that we need a newer conda version for that to work: conda/conda@f531272

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was doing activation manually in cases like that, which is horrendous, but it got the job done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, and another problem: conda build does not activate the environment but only changes the PATH: at least the gopath isn't set in during tests :-(

The following NEW packages will be INSTALLED:

    go: 1.4.2-0

Fetching packages ...
go-1.4.2-0.tar 100% |###############################| Time: 0:00:00 130.22 MB/s
Extracting packages ...
[      COMPLETE      ]|##################################################| 100%
Linking packages ...
[      COMPLETE      ]|##################################################| 100%

C:\portabel\miniconda\conda-bld\test-tmp_dir>go version
go: cannot find GOROOT directory: C:\portabel\miniconda\envs\_build\go

C:\portabel\miniconda\conda-bld\test-tmp_dir>if errorlevel 1 exit 1
TESTS FAILED: go-1.4.2-0

I'm currently thinking about this workaround:

  commands:
    - activate _test  # [win]
    - go version
    - go help
    - go run hello.go
    - cmd /c echo x%GOROOT% NEQ x%CONDA_DEFAULT_ENV%\go  # [win]
    - cmd /c if x%GOROOT% NEQ x%CONDA_DEFAULT_ENV%\go exit 1   # [win]

Copy link
Contributor

@jankatins jankatins Jun 3, 2016

Choose a reason for hiding this comment

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

And the workaround works :-) -> jakirkham#11

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I confused the two issues. Also, using activate to solve this problem on *NIXes. This is the conda-build issue ( conda/conda-build#910 ) that pertains to fixing this problem.

@jakirkham jakirkham force-pushed the add_python-toolchain branch 3 times, most recently from 7b8b25b to 013ca3f Compare June 6, 2016 14:51
@jakirkham
Copy link
Member Author

Seems that we are still suffering form activation issues on Windows. Suggestions?

@jankatins
Copy link
Contributor

Seems that we are still suffering form activation issues on Windows. Suggestions?

commands:
    - activate _test  # [win]
    - python tests.py # ot whatever...

And something similar in bld.bat?

That's a horrible workaround, but it seems to work :-)

number: 0
script:
- source activate _build # [unix]
- activate _build # [win]
Copy link
Member Author

Choose a reason for hiding this comment

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

So, I'm trying to activate the _build environment here, but that seems to not be setting PIP_CONFIG_FILE. Is there something obviously wrong here? While using CONDA_DEFAULT_ENV may be more robust, I would still expect for this to work on Windows. Am I still installing the scripts in the wrong location? Double-checked and thought this was ok, but I could still be wrong about this.

Copy link
Member

Choose a reason for hiding this comment

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

Nothing obvious. What happens if you just do

activate _build

Interactively?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. If I had a working Windows machine, I could tell you. 😉 Unfortunately, no such luck. 😞

Copy link
Member Author

Choose a reason for hiding this comment

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

Though I do have a sort of working Windows machine (think the compiler installs didn't go over well 😨). Fortunately, this doesn't require working compilers (I would hope 👼). FWICT it is not activating correctly there. 😕

Copy link
Member

Choose a reason for hiding this comment

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

Don't forget you can log into appveyor directly:
https://www.appveyor.com/docs/how-to/rdp-to-build-worker

On Mon, Jun 6, 2016 at 4:15 PM jakirkham [email protected] wrote:

In recipes/webcolors/meta.yaml
#642 (comment)
:

+{% set version = "1.5" %}
+
+package:

  • name: webcolors
  • version: {{ version }}

+source:

+build:

  • number: 0
  • script:
    • source activate _build # [unix]
    • activate _build # [win]

Though I do have a sort of working Windows machine (think the compiler
installs didn't go over well 😨). Fortunately, this doesn't require
working compilers (I would hope 👼). FWICT it is not activating correctly
there. 😕


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/conda-forge/staged-recipes/pull/642/files/013ca3f8d866a57c79724e8ecb99a0c4b38c326d#r65970568,
or mute the thread
https://github.com/notifications/unsubscribe/AACV-bkgYdk7ebR2-bae5D8HTPEdBksLks5qJI3qgaJpZM4Ih7wZ
.

@jakirkham
Copy link
Member Author

@msarahan, do you see anything wrong with the Windows activation here? I think the paths are all correct, but I could still be missing something. If you can see something that is obviously wrong, please let me know. As this would be something using PR ( conda/conda-build#1002 ), it would be good if we can make sure this is working correctly. Maybe we are running into common issues.

@jakirkham jakirkham force-pushed the add_python-toolchain branch from 013ca3f to 1b3f39c Compare June 6, 2016 21:29
@jankatins
Copy link
Contributor

jankatins commented Jun 6, 2016

I think I remember fixing my activate script as the currently released version of conda/conda-env does not have the activate.d step as part of the endlocal line (line = endlocal && (....) even if there are line breaks) and therefore nothing of what happens in the activate.d scripts is surviving until the activate script returns.

conda/conda@f531272

-> workaround:

[...]

endlocal & (
    REM Used for deactivate, to make sure we restore original state after deactivation
    SET "CONDA_PATH_BACKUP=%CONDA_PATH_BACKUP%"
    SET "CONDA_OLD_PS1=%CONDA_OLD_PS1%"
    set "PROMPT=%PROMPT%"
    set "PATH=%PATH%"
    set "CONDA_DEFAULT_ENV=%CONDA_NEW_ENV%"
    )

rem ---- NEW ---
REM Run any activate scripts outside of "setlocal"
IF EXIST "%CONDA_DEFAULT_ENV%\etc\conda\activate.d" (
    PUSHD "%CONDA_DEFAULT_ENV%\etc\conda\activate.d"
    FOR %%g in (*.bat) DO @CALL "%%g"
    POPD
)
rem ---- NEW ---

[...]

@jakirkham
Copy link
Member Author

Yeah, I was about to say I think this is why I had setlocal EnableDelayedExpansion added. I'm going to try bringing that back.

@jakirkham jakirkham force-pushed the add_python-toolchain branch from 1b3f39c to c2bbab5 Compare June 6, 2016 21:39
@jakirkham
Copy link
Member Author

Yeah, otherwise we see these really weird if not exist statements that weren't part of the original script.

@jakirkham
Copy link
Member Author

Nope, I'm wrong that makes no difference. Same problem.

@jankatins
Copy link
Contributor

as far as I understand batch, the only think to make it work would be to hack the activate.bat script.

@jankatins
Copy link
Contributor

Re the if not exist parts: aren't that the tests? if not exist "%LIBRARY_PREFIX%\\etc\\pip.conf" exit 1

commands:
- test -f "${PREFIX}/etc/pip.conf" # [unix]
#- test -f "${STDLIB_DIR}/distutils/distutils.cfg" # [unix]
- if not exist "%LIBRARY_PREFIX%\\etc\\pip.conf" exit 1 # [win]
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the appveyor log you don't need \\, \ is enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm a little more worried about something parsing the yaml and misinterpreting something. As AppVeyor can handle this fine, I'm happy enough to leave this alone.

@jakirkham
Copy link
Member Author

So, basically, we need a conda release before we can do anything with Windows really. IMHO we should not be asking people to add such a long chunk of stuff in bld.bat to effectively activate and deactivate things.

@jankatins
Copy link
Contributor

jankatins commented Jun 6, 2016

So, IMO currently (with the current conda build and conda/conda env) there are two issues which make this fail:

  • conda build does not activate the build environments, only sets the PATH and some other stuff, but doesn't get anything else from activate.d scripts (Conda needs to activate _build and _test environments - not monkey with PATH conda/conda-build#910)
  • but even if it would do this (or the tests use the workaround of activating the env actively), the activate.d scripts can't influence the environment as they are currently run inside the setlocal scope. (fixed in conda master, but apparently not yet released?)

@jakirkham
Copy link
Member Author

You're right. I guess the copying is buried in these lines. Maybe I should enable some sort of verbose flag so it is easier to read.

@jankatins
Copy link
Contributor

jankatins commented Jun 6, 2016

[burried lines]
That's actually something I also noticed: the bld.bat script is not anymore run with echo on?

@jakirkham jakirkham force-pushed the add_python-toolchain branch 2 times, most recently from 1e29b93 to fcfd155 Compare June 6, 2016 22:08
@jakirkham
Copy link
Member Author

I don't think I ever had echo on. Trying to add it now.

@jakirkham
Copy link
Member Author

Anyways conda 4.1 and conda-build 2.0.0 are on there way. We know half the problem is solved already. We just need to solve the other half.

@jakirkham
Copy link
Member Author

It might be time to pick this up again as the needed changes for Windows are in conda/conda-build.

@jakirkham jakirkham force-pushed the add_python-toolchain branch from fcfd155 to 817bf2a Compare July 25, 2016 03:44
@jakirkham
Copy link
Member Author

Rebased on current master. Though we may still have some issues with AppVeyor and activation scripts.

@jakirkham jakirkham force-pushed the add_python-toolchain branch from 817bf2a to 6e185ea Compare August 19, 2016 19:23
@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 (recipes/python-toolchain) and found it was in an excellent condition.

@jakirkham jakirkham force-pushed the add_python-toolchain branch from 6e185ea to 7a496af Compare August 19, 2016 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Python install line standard
6 participants