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

Replace orderedset with ordered_set #106

Merged
merged 3 commits into from
Apr 10, 2022
Merged

Replace orderedset with ordered_set #106

merged 3 commits into from
Apr 10, 2022

Conversation

wom-bat
Copy link
Member

@wom-bat wom-bat commented Apr 6, 2022

The orderedset PyPi package no longer works with Python 3.10,
and has not been updated for years.

Replace it with the functionally equivalent (but maintained) ordered-set
package.

Signed-off-by: Peter Chubb peter.chubb@unsw.edu.au

The orderedset PyPi package no longer works with Python 3.10,
and has not been updated for years.

Replace it with the functionally equivalent (but maintained) ordered-set
package.

Signed-off-by: Peter Chubb <peter.chubb@unsw.edu.au>
@wom-bat wom-bat requested review from axel-h and ikuz April 6, 2022 03:49
@wom-bat
Copy link
Member Author

wom-bat commented Apr 6, 2022

When/if this gets merged camkes-deps package on PyPi will have to be updated.

@wom-bat
Copy link
Member Author

wom-bat commented Apr 6, 2022

I expect builds will fail as the docker images won't use the new CAmkES dependency.

Bump the version so pip upgrade will find the new ordered-set
dependency.

Signed-off-by: Peter Chubb <peter.chubb@unsw.edu.au>
@lsf37
Copy link
Member

lsf37 commented Apr 6, 2022

No problems from my side. Are there other repos who use the old dependency that we should also update?

@lsf37
Copy link
Member

lsf37 commented Apr 6, 2022

No problems from my side. Are there other repos who use the old dependency that we should also update?

To answer my own question: according to github search the only other mentions are in capdl/python-capdl-tool/requirements.txt and in docs/projects/camkes/next.md. It seems unused in capdl, so maybe should be removed there.

@axel-h
Copy link
Member

axel-h commented Apr 6, 2022

This seems the minimum fix, looks ok to me.

Since Python 3.6 he core functionality dict() is also guaranteed to keep the order. Might be worth using this one day to get rid of a external dependency,

@axel-h
Copy link
Member

axel-h commented Apr 6, 2022

Background about the Python 3.10 bug: simonpercivall/orderedset#34 and simonpercivall/orderedset#36, switching to ordered-set is also recommended there.

The method for uploading packages to PyPI has changed; document the
current method in setup.py, plus add the now desirable description
field, and an organisation-independent email address.

Signed-off-by: Peter Chubb <peter.chubb@unsw.edu.au>
@wom-bat wom-bat merged commit 4f7bca1 into master Apr 10, 2022
@wom-bat wom-bat deleted the ordered_set branch April 10, 2022 23:49
@@ -39,9 +45,17 @@
name='camkes-deps',
version='0.7.2',
Copy link
Member

Choose a reason for hiding this comment

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

This version should get a minor bump, otherwise any python installation that's already installed it won't update to the version with ordered-set without --force-reinstall

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that would have been better. #107 does bump it, I think that'll be fine overall.

Copy link
Member Author

Choose a reason for hiding this comment

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

The version on PiPY was 0.7.1 so I figured 0.7.2 would be OK.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense, pypi won't let you overwrite old versions (at least not easily, afaik). Looks like Siwei only updated the repo, but not pypi when he bumped that version. I think the public state is fine, although maybe the repo history is not fully consistent with when 0.7.2 actually was published, but I don't think that's a problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Version 0.7.2 with these changes is on PyPI now, (with the exception of the whitespace changes Gerwin pushed)

Copy link
Member

Choose a reason for hiding this comment

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

Odd, I did a camkes-deps pip install after this PR was merged and 0.7.2 was already installed for me without ordered-set. --force-reinstall was required to get ordered-set. I agree that bumping to 7.3 in #107 is fine overally

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.

4 participants