mtkennerly / dunamai

Dynamic versioning library and CLI
https://dunamai.readthedocs.io/en/latest
MIT License
312 stars 24 forks source link

Style is overwritten which causes validation of format regardless if user passed parameter #59

Closed Hypeg closed 1 year ago

Hypeg commented 1 year ago

https://github.com/mtkennerly/dunamai/blob/fe69436133d0ea2097a9fed1dbb31ec96263cf83/dunamai/__init__.py#L678 https://github.com/mtkennerly/dunamai/blob/fe69436133d0ea2097a9fed1dbb31ec96263cf83/dunamai/__init__.py#L699 https://github.com/mtkennerly/dunamai/blob/fe69436133d0ea2097a9fed1dbb31ec96263cf83/dunamai/__init__.py#L738 https://github.com/mtkennerly/dunamai/blob/fe69436133d0ea2097a9fed1dbb31ec96263cf83/dunamai/__init__.py#L1788

Currently using poetry to generate version dynamically(poetry-dynamic-versioning). If Format is configured but style isn't, Format is still validated due to being overwritten on both line 678 and during method on 1788 parameter initialization. Method call on 738 never gets called as value is set before if statement on 699.

I suggest you either remove 678 to allow 699 to be called and set default value of style parameter in 1788 to None. Or you could add an additional conditional to 678 to check if format is set to then configure the style and then set default value of style parameter in 1788 to None.

Hopefully no upstream issues by value being none. Love the plugin Thanks

mtkennerly commented 1 year ago

If Format is configured but style isn't, Format is still validated due to being overwritten

I think you have that backwards. When format is set, there's an early return. This is before the style = Style.Pep440 line, so the style is only enforced when explicitly passed:

https://github.com/mtkennerly/dunamai/blob/fe69436133d0ea2097a9fed1dbb31ec96263cf83/dunamai/__init__.py#L673-L675

$ git tag
v1.2.3x4

$ dunamai from git --format "{base}{stage}{revision}"
1.2.3x4

$ dunamai from git --format "{base}{stage}{revision}" --style pep440
Version '1.2.3x4' does not conform to the PEP 440 style

When format is not set, then the style is always enforced. I think that's in line with its description: "Will default to PEP 440 if not set and no custom format given".

$ dunamai from git
Version '1.2.3x4' does not conform to the PEP 440 style

Could you tell me about your use case? Why do you want the default PEP 440 serialization without enforcing the style? Even if I add an option to disable the validation, you'll still have to deal with Poetry's built-in validation as well. Actually, if you're using format without style, then maybe you're seeing an error from Poetry itself rather than from Dunamai?

Hypeg commented 1 year ago

Hi, Apologies I didn't write that out perfectly. I had meant that style was being overwritten and it caused issues with my format.

My use case is I currently am using azure Devops in a ci/cd pipeline to automatically run multiple functions on code I am committing. Linting, static type checking, unit tests and in the end I am publishing my wheel I am generating to a pypi azure devops feed. Currently I am needing a better way to organize wheels that get generated from commits that potentially all have the same version. As my pipeline will generate one for each that pass all the requirements in the pipeline. Because of this I wanted to use the pep440 standard with hashes from my commits. However due to limitations on azure devops side I have to drop some special characters. So I had tried to go in and preset a format that had the same information but didn't use a '+' for instance. Since I didn't set a style and I just wanted the constructed version it creates, it fails since it didn't follow pep440 standards. The documentation for the plugin had said it wouldn't validate if I didn't set style so I just wanted to track down the cause.

Hopefully this makes sense, Thanks again

On Thu, Mar 2, 2023, 10:15 PM Matthew Kennerly @.***> wrote:

If Format is configured but style isn't, Format is still validated due to being overwritten

I think you have that backwards. When format is set, there's an early return. This is before the style = Style.Pep440 line, so the style is only enforced when explicitly passed:

https://github.com/mtkennerly/dunamai/blob/fe69436133d0ea2097a9fed1dbb31ec96263cf83/dunamai/__init__.py#L673-L675

$ git tag v1.2.3x4

$ dunamai from git --format "{base}{stage}{revision}" 1.2.3x4

$ dunamai from git --format "{base}{stage}{revision}" --style pep440 Version '1.2.3x4' does not conform to the PEP 440 style

When format is not set, then the style is always enforced. I think that's in line with its description: "Will default to PEP 440 if not set and no custom format given".

$ dunamai from git Version '1.2.3x4' does not conform to the PEP 440 style

Could you tell me about your use case? Why do you want the default PEP 440 serialization without enforcing the style? Even if I add an option to disable the validation, you'll still have to deal with Poetry's built-in validation as well.

— Reply to this email directly, view it on GitHub https://github.com/mtkennerly/dunamai/issues/59#issuecomment-1452897314, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEK3ANX6N4CD7FUH3G3TSKTW2FO4NANCNFSM6AAAAAAVOANFNY . You are receiving this because you authored the thread.Message ID: @.***>

Hypeg commented 1 year ago

This was the set of documentation from the poetry-dynamic-versioning that implements this code.

"style (string, default: unset): One of: pep440, semver, pvp. These are preconfigured output formats. If you set both a style and a format, then the format will be validated against the style's rules. If style is unset, the default output format will follow PEP 440, but a custom format will only be validated if style is set explicitly. " <- This last part here

Also in regards to "maybe you're seeing an error from Poetry itself rather than from Dunamai?"

The error message I received was being set on this line: https://github.com/mtkennerly/dunamai/blob/fe69436133d0ea2097a9fed1dbb31ec96263cf83/dunamai/__init__.py#L1800

mtkennerly commented 1 year ago

Could you share your poetry-dynamic-versioning config and the full error message (including the version that it doesn't like)?

mtkennerly commented 1 year ago

Are you maybe using poetry-dynamic-versioning's format-jinja option and then calling serialize_pep440 in the Jinja code? That would enforce the PEP 440 style even though you have a custom format.

Hypeg commented 1 year ago

I will have to get back to you on this one as I am not near my work computer at the moment. I think this could potentially just be an issue with poetry's backend also enforcing it. If that is the case is there a way to stop this?

From my understanding of this documentation I posted earlier the style should change the output. With some testing on my personal computer it certainly does. If I set the format to "v{base}-{distance}.{commit}" & the style to "pvp" the output is "v0.1.0-5.f74e3d2". however this throws the error "Version 'v0.1.0-5.f74e3d2' does not conform to the PVP style". If I then remove the format and only use style which is still set to "pvp" this generates the same output but gives me this error "invalid PEP 440 version: 'v0.1.0-5.f74e3d2'. Is this the poetry error?

This doesnt make too much sense to me though as to why it failed the format that I was enforcing as pvp but still clearly generates the same output when I remove the format. The normal pep440 output certainly succeeds if I just replace the dash with a +: "v{base}+{distance}.{commit}. So it most certainly is changing it.

In the end my conclusion is this from what I can see If Style + Format set. If Format != anyStyleRegex == failure If Not Style + Format set. If Format != pep440Regex == failure due to base poetry not meeting pep440 If Style + Not Format set. Format != pep440Regex == failure due to base poetry not meeting pep440, except for semver If Style == pvp and Format set. Even if formatOutput == styleOutput. Format != pvpRegex == failure.

Am I just doing something wrong? It seems like setting the format by itself will never work if you dont meet pep440 standards in which case its pointless?

Apologies, I thought I had tracked down the issue but it seems ive just gone down a rabbit hole.

mtkennerly commented 1 year ago

If I then remove the format and only use style which is still set to "pvp" this generates the same output but gives me this error "invalid PEP 440 version: 'v0.1.0-5.f74e3d2'. Is this the poetry error?

Yes, that error (invalid PEP 440 version) is coming from Poetry Core here: https://github.com/python-poetry/poetry-core/blob/ea1881305ae8ef5b40203b4f214a386ddecd90cb/src/poetry/core/version/pep440/parser.py#L71

If I set the format to "v{base}-{distance}.{commit}" & the style to "pvp" the output is "v0.1.0-5.f74e3d2". however this throws the error "Version 'v0.1.0-5.f74e3d2' does not conform to the PVP style".

This error (does not conform) comes from Dunamai, since PVP doesn't allow a - segment followed by a . segment. For example:

The leading v is also invalid for PVP. (v in tag -> okay, but v in format -> not okay)

Am I just doing something wrong? It seems like setting the format by itself will never work if you dont meet pep440 standards in which case its pointless?

More or less, yeah, you still need to satisfy Poetry's validation of the version. You're right that it makes the setting less useful, but there are some cases where it can work (e.g., I think Poetry can massage some SemVer output into something compatible).

I can try an experiment to see what happens if the plugin were to bypass Poetry's validation (if that's even possible), but I'm not sure what side effects it might have.

Hypeg commented 1 year ago

Okay Alright. I just don't know how to meet their format exactly to have hashes but also not include a '+'. Every attempt i did to replace it with something else or move it around just failed but I'll certainly see what I can do. Its certainly a pain that I cant use anything other than "a-zA-Z0-9._" only in azure devops naming, seeing as though something in poetry is also denying me.

Appreciate the help though, ill look into how I can just get poetry to accept it.

mtkennerly commented 1 year ago

I had an issue like that once before with a certain proprietary package type that didn't support letters in the version. It's not perfect, but I ended up converting the hashes from base 16 to base 10 and using them like a normal part of the version number. In your case, 0.1.0+5.f74e3d2 would become 0.1.0.5.259318738. Would that work for you?

From a preliminary test, even if I avoid that Poetry Core parse function and construct the class directly, I can only force Poetry to use a preformatted invalid version for certain logging. The artifact names are still generated in a way that would be much harder for the plugin to change.

Hypeg commented 1 year ago

I am not sure that would be the best option for me. I think it would be best to just use it just without including the hash and just inject that separately as to avoid the issues with not meeting the pep. Or I was thinking perhaps is it possible to overwrite which commands the process uses the version? The environmental variable below seems like it could accomplish this. If I perhaps only say have it activate during poetry version -s and have the list only include that command will it then bypass it during the build or publish stage? If it does I can just use it as pvp generate the output then manually rename the file after it builds. I am sure I can find a way around it poetentially but its unfortunate poetry forces the validation.

POETRY_DYNAMIC_VERSIONING_COMMANDS: You can set a comma-separated list of Poetry commands during which to activate the versioning. For example, build,publish will limit the dynamic versioning to those two commands.

mtkennerly commented 1 year ago

Renaming the wheel could work. Have you tried just letting poetry build generate the wheel name with +, and then renaming the file to replace + with -? If Azure only cares about the file name, then that should work.

Hypeg commented 1 year ago

That certainly could be a solution but doing any string manipulation in azure would be more difficult than just replacing it outright. Their tool certainly isn't the most flexible

On Fri, Mar 3, 2023, 4:03 PM Matthew Kennerly @.***> wrote:

Renaming the wheel could work. Have you tried just letting poetry build generate the wheel name with +, and then renaming the file to replace + with -? If Azure only cares about the file name, then that should work.

— Reply to this email directly, view it on GitHub https://github.com/mtkennerly/dunamai/issues/59#issuecomment-1454126012, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEK3ANXRHT4W3OAVUZRFJ5LW2JMBPANCNFSM6AAAAAAVOANFNY . You are receiving this because you authored the thread.Message ID: @.***>

mtkennerly commented 1 year ago

Hmm, I'm not familiar with Azure DevOps, but doesn't it have a way to run arbitrary shell scripts so you could use sed or an equivalent?

Hypeg commented 1 year ago

Yeah you can run shell scripts but doesnt sed operate on information in a file not on the file name itself? In the end all I would need is to know the information I want to replace it with and the original file name. I use mv in order to replace the filename with something else. Seems to be the quickest method since all I need is to just rename the wheel before I publish it. Not too familiar with unix commands though.

mtkennerly commented 1 year ago

sed can operate on any text that you pipe into it. Here's an example:

for filename in ./*.whl; do
    new_filename=$(echo "$filename" | sed s/+/-/g)
    mv "$filename" "$new_filename"
done