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

DM-27354: Update for sphgeom pip install #423

Merged
merged 2 commits into from Nov 5, 2020
Merged

DM-27354: Update for sphgeom pip install #423

merged 2 commits into from Nov 5, 2020

Conversation

jonathansick
Copy link
Member

sphgeom no longer has a requirements.txt file (as of lsst/sphgeom#30), so this PR removes the step from the build_and_test workflow that executes such a requirements.txt file.

Secondly, the install_requires dependency listing in setup.cfg now installs sphgeom from GitHub, making the sphgeom installation from a requirements.txt file unnecessary.

Note that this PR's build is expected to fail until the sphgeom PR is merged. Both PRs will need to be merged in quick succession.

@@ -2,7 +2,7 @@ pyyaml >= 5.1
astropy >= 4.0
click >= 7.0
sqlalchemy >= 1.3
https://github.com/lsst/sphgeom/archive/master.tar.gz
git+git://github.com/lsst/sphgeom@master#egg=lsst_sphgeom
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 better than the previous URL?

Copy link

@r-owen r-owen Nov 3, 2020

Choose a reason for hiding this comment

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

I spent some time trying to find documentation about this. The following is the best link I found: https://pip.pypa.io/en/stable/reference/pip_install/#requirement-specifiers

and it pointed me to the following for the various git protocols: https://git-scm.com/book/en/v2/Git-on-the-Server-The-Protocols
If I read this correctly I think the main advantage to git+git is speed. It can only be used for unauthenticated read, which is fine for this use case. The "cons" section of that document talks about the fact that the port is often blocked by corporate firewalls, which is worrisome.

@timj
Copy link
Member

timj commented Nov 5, 2020

We've done the big push so I'm fine with the sphgeom PR and this PR being merged at any time.

sphgeom no longer has a requirements.txt file, so this commit removes
the step from the build_and_test workflow that executes such a
requirements.txt file.

Secondly, the install_requires dependency listing in setup.cfg now
installs sphgeom from GitHub, making the sphgeom installation from a
requirements.txt file unnecessary.
Previously we got them implicitly.
@timj timj merged commit 5126caa into master Nov 5, 2020
@timj timj deleted the tickets/DM-27354 branch November 5, 2020 16:22
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.

None yet

3 participants