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

CI Should pip install from the repo #1781

Closed
2 tasks
chayim opened this issue Dec 9, 2021 · 7 comments · Fixed by #1790
Closed
2 tasks

CI Should pip install from the repo #1781

chayim opened this issue Dec 9, 2021 · 7 comments · Fixed by #1790
Assignees
Labels
good first issue maintenance Maintenance (CI, Releases, etc)

Comments

@chayim
Copy link
Contributor

chayim commented Dec 9, 2021

Currently, our CI builds packages and runs unit tests against the build package. However, we don't try to pip install directly from the repo - another valid install case. This has been highlighted by bug #1625, and it's multiple fixes. Ideally the .github/workflows/integration.yaml would change, to also trigger a pure pip install of the package. Usage of other tools (i.e conda?) would make sense as well.

Actions:

  • Add another CI action run to build the packages and pip install (using the current git commit))
  • Add for tools such as ....
@chayim chayim added good first issue maintenance Maintenance (CI, Releases, etc) labels Dec 9, 2021
@ashwani99
Copy link
Contributor

Hi @chayim can I pick this?

@hartwork
Copy link
Contributor

hartwork commented Dec 9, 2021

I'm not sure if pip install can be tested in CI, the way you likely intend to. If I do pip install git+file://${PWD}/, pip seems to ignore setup_requires. Same for pip install git+https://github.com/redis/redis-py. And I guess it has to, because how is pip supposed to get to the value in setup_requires without running setup.py which already needs packaging around: hello chicken egg loop. My guess is this will only ever work for packages uploaded and installed from PyPI, rather than locally. Can you think of another way to confirm or falsify that assumption other than uploading 4.0.3 to PyPI?

@hartwork
Copy link
Contributor

hartwork commented Dec 9, 2021

PS: For reference: https://pip.pypa.io/en/stable/reference/build-system/#controlling-setup-requires

@chayim
Copy link
Contributor Author

chayim commented Dec 9, 2021

@hartwork I wouldn't do a git+file install, but git+https:// which should help. The reality is that we have tests for the packages we build - each in their own environment. But, we don't have tests for the repo code itself, which I think is important! At the very least, this helps improve things, another step.

@chayim
Copy link
Contributor Author

chayim commented Dec 9, 2021

Thanks @ashwani99. Give it a go!

@hartwork
Copy link
Contributor

hartwork commented Dec 9, 2021

@hartwork I wouldn't do a git+file install, but git+https:// which should help. The reality is that we have tests for the packages we build - each in their own environment. But, we don't have tests for the repo code itself, which I think is important! At the very least, this helps improve things, another step.

@chayim I don't feel my point at #1781 (comment) above has been heard. Are you aware of the limitations of setup_requires?

PS: For git+https:// URLs, you would need to include the very commit SHA1 into the URL as well (unlike with a file URL) but that is independent of the setup_requires issue).

@chayim
Copy link
Contributor Author

chayim commented Dec 12, 2021

@hartwork I think we're on the same page. Yes, we'll need our CI to inject the commit hash and branch (available in ${GITHUB_SHA} and ${GITHUB_REF} respectively). This does not solve that limitation - but it does help move the ball with regards to other testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue maintenance Maintenance (CI, Releases, etc)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants