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 pip/poetry for submodules, better pep440 #114

Closed
wants to merge 1 commit into from

Conversation

marius311
Copy link
Contributor

Couple small things that I only caught after #97 got merged into the private repo and I used in action.

  1. The private repo has tags like "autoprocessing-3.3" so the version string created wasn't PEP440 compliant (unlike this public repo which just has number tags). The solution in this PR is to strip out starting characters. Open to other suggestions.

  2. -e instead of -d works if the repo is a submodule, in which case .git is a file not directory.

@nwhitehorn
Copy link
Member

Is "autoprocessing-0.3" actually equivalent to "somethingelse-0.3"?

@arahlin
Copy link
Member

arahlin commented Aug 21, 2023

Is "autoprocessing-0.3" actually equivalent to "somethingelse-0.3"?

No, it's not. But we do also store the full tag name, and the autoprocessing version actually gets recorded as a separate variable altogether, so it doesn't really matter for this purpose.

@marius311
Copy link
Contributor Author

So what if anything do I need to do?

@nwhitehorn
Copy link
Member

nwhitehorn commented Aug 22, 2023

Well, I'm not sure what this accomplishes or what is being fixed.

  • If the version strings are supposed to be unique, this makes them not be.
  • If they aren't supposed to be unique, what are they for?

So it doesn't seem like this can work. Am I missing something? Is the real fix that we need to change how we assign versions?

@nwhitehorn
Copy link
Member

So here is my proposed solution:

  • We don't commit this patch. I don't think it makes sense to declare the software version to be whatever numbers happen to be contained in the most recent git tag.
  • We pick a new format for the tags. I would propose we use the local-version-identifier bits in PEP440 to make the SPT-private version strings be +. For example, to make "autoprocessing-v3.3" be "0.3+autoprocessing-3.3", which is almost PEP-440 compliant. This code could replace hyphens with dots to make it fully PEP-440 compliant. Or we could do that in the tag.
  • We should probably make a new release of the public repo, which hasn't had one since October 2020.

@arahlin
Copy link
Member

arahlin commented Aug 22, 2023

So here is my proposed solution:

  • We don't commit this patch. I don't think it makes sense to declare the software version to be whatever numbers happen to be contained in the most recent git tag.
  • We pick a new format for the tags. I would propose we use the local-version-identifier bits in PEP440 to make the SPT-private version strings be +. For example, to make "autoprocessing-v3.3" be "0.3+autoprocessing-3.3", which is almost PEP-440 compliant. This code could replace hyphens with dots to make it fully PEP-440 compliant. Or we could do that in the tag.
  • We should probably make a new release of the public repo, which hasn't had one since October 2020.

This seems like a reasonable solution to me. If we're going that route, I'd prefer that we make PEP440-compliant tags rather than write code to make them so.

I think we still need the -e change for detecting the presence of the .git file/directory, so that part of this PR should remain.

@arahlin arahlin self-requested a review August 23, 2023 21:18
arahlin added a commit that referenced this pull request Feb 20, 2024
@arahlin
Copy link
Member

arahlin commented Feb 20, 2024

@marius311 are you ok with the plan discussed above? If so, I'll close this PR.

@marius311
Copy link
Contributor Author

That's fine by me. Anything else you need from me or you guys will handle the remaining work?

@arahlin
Copy link
Member

arahlin commented Feb 27, 2024

That's fine by me. Anything else you need from me or you guys will handle the remaining work?

I think the rest of the work is on us. Thanks!

@arahlin arahlin closed this Feb 27, 2024
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.

3 participants