Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Freeze to django-mptt version to fix set+ tuple error #86

Merged
merged 5 commits into from Jan 26, 2018

Conversation

mustyoshi
Copy link
Contributor

It took me spinning up two fresh machines to narrow this down.

@mustyoshi mustyoshi closed this Jan 24, 2018
@mustyoshi mustyoshi reopened this Jan 24, 2018
@coveralls
Copy link

coveralls commented Jan 24, 2018

Coverage Status

Coverage remained the same at 93.703% when pulling 231f359 on mustyoshi:master into d544333 on eregs:master.

Copy link
Member

@cmc333333 cmc333333 left a comment

Choose a reason for hiding this comment

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

Thanks for finding this, @mustyoshi! I think we'll need to expand the supported requirements a bit, but otherwise we can get this in and released ASAP.

setup.py Outdated
@@ -8,8 +8,8 @@
include_package_data=True,
install_requires=[
'cached_property',
'django>=1.8,<1.12',
'django-mptt',
'django==1.11',
Copy link
Member

Choose a reason for hiding this comment

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

I suspect you didn't intend to pin to exactly 1.11 (which excludes 1.11.1, 1.11.2, etc.). As a best effort, we should also try to support 1.8 through 1.10 until we hit an incompatibility. I think that means we can revert this line?

setup.py Outdated
'django>=1.8,<1.12',
'django-mptt',
'django==1.11',
'django-mptt==0.8.7',
Copy link
Member

Choose a reason for hiding this comment

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

Sounds like 0.9 caused the issue, so we'll want to exclude that. For maximum compatibility with downstream projects, though, we'll want to expand this version span. Does django-mptt~=0.8.3 work? That's should match 0.8.3 (the version available when we added this dependency) through 0.8.7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll test and make another commit tonight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So on my ubuntu 16.04 machine, running Python 3.6

This is just building the docker image
docker build . -t eregs/core

django==1.9 works on mptt>=0.8.3,<=0.8.7
django==1.10 works on mptt>=0.8.6,<=0.8.7
django==1.11 works on mptt>=0.8.6,<=0.8.7

I'm not too familiar with Python setuptools, is there a way to make conditional library versioning?

Copy link
Member

Choose a reason for hiding this comment

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

Womp womp. Thanks so much for testing all of that! Looks like we'll get the max coverage with

django >= 1.10, <1.12
django-mptt ~= 0.8.6

I've opened #87 for removing mptt going forward.

@cmc333333 cmc333333 mentioned this pull request Jan 25, 2018
@cmc333333
Copy link
Member

Thanks so much @mustyoshi !

@cmc333333 cmc333333 merged commit 0b2a203 into eregs:master Jan 26, 2018
vrajmohan pushed a commit to fecgov/fec-eregs that referenced this pull request Apr 10, 2018
@johnnyporkchops
Copy link

@mustyoshi , At FEC, we forked the eregs/regluations-core repo and tried to update python packages like Django and django-mptt.

  • Migrations only work with django-mptt 0.8.6 on fec-eregs repo. regcore.0003_mptt_copy_children fails with any later version of django-mptt.
  • When we upgrade django-mptt (0.9.0/0.11.0) on regulations-core, migrations work.
    The problem is that on fec-eregs migrations continue to fail with the set+ tuple error that you fixed with this PR.

We are stumped! Since you worked on this before, do you know what the nature of the error is and can you think of any possible workarounds?

@mustyoshi
Copy link
Contributor Author

@johnnyporkchops, unfortunately I am not very familiar with the django/django-mptt ecosystem, I had only been messing around with this repo.
I did some testing, and if you keep the django-mptt version at 0.8.6, the docker image will finish building. I was also able to start the webserver.

cmc was in the process of depreciating the use of django-mptt, so you may have to just live with the outdated version. The good news is that django@2.2.11 does work with django-mptt@0.8.6

Sorry I couldn't be more helpful.

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

Successfully merging this pull request may close these issues.

None yet

4 participants