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

[BUG] + in version string breaks fingerprint system #995

Closed
anders-kiaer opened this issue Nov 3, 2019 · 2 comments · Fixed by #999
Closed

[BUG] + in version string breaks fingerprint system #995

anders-kiaer opened this issue Nov 3, 2019 · 2 comments · Fixed by #999
Assignees
Milestone

Comments

@anders-kiaer
Copy link
Contributor

Describe your context

  • replace the result of pip list | grep dash below
dash                                  1.5.1                           
dash-core-components                  1.4.0                           
dash-daq                              0.2.2                           
dash-html-components                  1.0.1                           
dash-renderer                         1.2.0                           
dash-table                            4.5.0  

Describe the bug

When going from dash==1.4 to dash==1.5, we experienced a breaking change in the custom Dash components we use.

It took some hours to debug, but the reason was found to be related to the new "fingerprint" system in Dash. In our project, we use the setuptools_scm package (by the Python Packaging Authority) in order to have a versioning system that automatically is linked to the git repo tags. This makes continuous deployment to e.g. Pypi easy and robust wrt. keeping versions consistent.

I.e. instead of

__version__ = package['version']

in the component package, we use something like

__version__ = get_distribution(__name__).version

This worked until dash==1.5, then it broke on non-release-versions due to automatic tags of the type
1.0.0.dev5+af4304c.d20191103, where the tag includes a +. See the default tag formats.

Changing the line above to

__version__ = get_distribution(__name__).version.replace("+", ".")

is one workaround that gets the third party components to also work on dash==1.5

Expected behavior

setuptools_scm provided versions to work also in dash>=1.5.

Suggested solution

Change this line in Dash's build_fingerprint to also replace + with _?

@anders-kiaer anders-kiaer changed the title [BUG] [BUG] + in version string breaks fingerprint system Nov 3, 2019
@alexcjohnson
Copy link
Collaborator

Thanks @anders-kiaer

@Marc-Andre-Rivet we could go for full symmetry with cache_regex:

cache_regex = re.compile(r"^v[\w-]+m[0-9a-fA-F]+$")  # unchanged
version_clean = re.compile(r"[^\w-]")


def build_fingerprint(path, version, hash_value):
    path_parts = path.split("/")
    filename, extension = path_parts[-1].split(".", 1)

    return "{}.v{}m{}.{}".format(
        "/".join(path_parts[:-1] + [filename]),
        re.sub(version_clean, "_", str(version)),
        hash_value,
        extension,
    )

unless you can think of a scenario where we end up with matching cleaned strings for different versions... then I guess we'd need to somehow escape these chars rather than just setting them all to _.

@Marc-Andre-Rivet
Copy link
Contributor

The metadata should be the last of them..

major.minor.patch: https://semver.org/#spec-item-2
pre-release: https://semver.org/#spec-item-9
metadata: https://semver.org/#spec-item-10

@alexcjohnson 👍 for the suggested change above. In theory it would be possible to generate the same result string by appending the metadata to the pre-release string with . instead of + -- we could transform + instead two consecutive _ to be safe, _ not being allowed in either of the pre-release or metadata strings, same for consecutive ..

The paranoid version becomes:

str(version).replace(".", "_").replace("+", "__")

which is what I'm going for.

Will also need to update https://github.com/plotly/dash/blob/dev/@plotly/webpack-dash-dynamic-import/src/index.js#L8 - neither the DataTable or DCC use metadata in their release versions, so only the plugin needs to have a patch release. We can update usage across the board in the next minor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants