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

fix: use virtualenv manually instead of pipx for renku install #187

Merged
merged 1 commit into from
Sep 9, 2021

Conversation

ableuler
Copy link
Contributor

@ableuler ableuler commented Sep 8, 2021

This PR is a reaction to a breaking change of setuptools which in turn broke our builds as pipx just pullst the latest of setuptools on a regular basis. We have therefore decided to remove pipx as it makes for non-reproducible builds.

@@ -61,8 +61,12 @@ ENV RENKU_DISABLE_VERSION_CHECK 1

ENV PATH=$HOME/.local/bin:$PATH

RUN pipx install --pip-args="--no-cache" renku && \
pipx inject --pip-args="--no-cache" renku sentry-sdk
RUN mkdir -p $HOME/.local/bin && \
Copy link
Member

Choose a reason for hiding this comment

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

If the user were to install renku as a library with pip install --user renku - would the executable go to the same place? What happens in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would overwrite it - bug or feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ie replace the symbolic link with the actual executable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes me think - should we give the user's version precedence, even without the --user flag? we could do this by appending $HOME/.local/bin to the PATH rather than prepending it.

Copy link
Member

Choose a reason for hiding this comment

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

I think overwriting it and using the version coming from the pip install is what you would expect (then when you do import renku it's importing the same code as the CLI is using). Anything else would be wrong. I just wasn't sure if it would throw an error of any kind.

Copy link
Member

Choose a reason for hiding this comment

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

the user won't be able to install it without the --user flag... afaik

Copy link
Contributor Author

@ableuler ableuler Sep 8, 2021

Choose a reason for hiding this comment

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

right... at least not inside a running session. but by adding it to the project requirements.txt they can. In that case, conda will actually update the path again during the install and prepend its location, which makes the path look like

/opt/conda/bin:/opt/conda/condabin:/home/jovyan/.local/bin:/opt/conda/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

which gives the conda-installed version precedence. I'd have the tendency to

  • create the symbolic link to location where it won't be overwritten
  • add this location to the end of the path such that user-installed renku versions always win.

wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

I think that makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ableuler ableuler force-pushed the 000-fix-setuptools branch 2 times, most recently from 430207d to cafec1e Compare September 8, 2021 20:16
@ableuler ableuler marked this pull request as ready for review September 8, 2021 20:20
@ableuler ableuler merged commit ba6cccc into master Sep 9, 2021
@ableuler ableuler deleted the 000-fix-setuptools branch September 9, 2021 09:16
TaoSunVoyage added a commit that referenced this pull request Oct 12, 2021
#187 changed where renku is installed.

renku is now in either `$HOME/.renku/bin` or `/share/bin` (batch)
TaoSunVoyage added a commit that referenced this pull request Nov 15, 2021
#187 changed where renku is installed.

renku is now in either `$HOME/.renku/bin` or `/share/bin` (batch)
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.

2 participants