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

support github vs. python product capitalization differences #173

Merged
merged 3 commits into from
Jun 4, 2021

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Jun 3, 2021

This PR fixes #172 by supporting installing products where the github name has a different capitalization than the python package name, e.g. https://github.com/desihub/QuasarNP hosting quasarnp. Tested with

desiInstall -v -r $SCRATCH/desi/test QuasarNP 0.1.0

(that outputs to NERSC /global/cscratch1/sd/sjbailey/desi/test, but I suggest running it yourself for testing...)

@sbailey sbailey requested a review from weaverba137 June 3, 2021 22:34
Copy link
Member

@weaverba137 weaverba137 left a comment

Choose a reason for hiding this comment

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

Perhaps I caught this PR in a preliminary stage, but there's a lot left to do here.

if (exists(join(working_dir, 'setup.py')) and isdir(join(working_dir, product))):
if (exists(join(working_dir, 'setup.py')) and
(isdir(join(working_dir, product)) or
isdir(join(working_dir, product.lower())))
Copy link
Member

Choose a reason for hiding this comment

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

As I mentioned in #172, these are not the only possible combinations. This change also needs to be applied to the case where the package lives in a py/ directory. Finally, we need unit tests for all of these cases.

@sbailey
Copy link
Contributor Author

sbailey commented Jun 4, 2021

I expanded a unit test for the new case where the product name is Capitalized by the python package isn't, e.g. QuasarNP/quasarnp, so these two cases are now supported and tested:

  • capitalization of the product and the python package agree (most of our products)
  • the product is Capitalized but the python package in the top level directory is all lowercase (e.g. QuasarNP/quasarnp)

However, I'm going to push back about adding support for these additional cases:

  • python package under py/ whose capitalization doesn't match the GitHub product name
  • GitHub product and python package having different capitalizations but the python package isn't entirely lower case

We don't have any cases that match either of those right now, and I don't anticipate having those cases in the future. I suggest that this PR adds new needed functionality without breaking old functionality, even if it doesn't additionally support all possible pathological future cases.

@weaverba137
Copy link
Member

OK, I see your point. I will merge and tag soon.

@weaverba137 weaverba137 merged commit 21c3de0 into master Jun 4, 2021
@weaverba137 weaverba137 deleted the InStall branch June 4, 2021 03:46
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.

desiInstall creates incorrect module file for QuasarNP
2 participants