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

[Issue 7050][python] Limit enum34 installation via environment markers #8213

Merged
merged 1 commit into from
Oct 13, 2020
Merged

[Issue 7050][python] Limit enum34 installation via environment markers #8213

merged 1 commit into from
Oct 13, 2020

Conversation

languitar
Copy link
Contributor

Fixes #7050

Motivation

To prevent installing enum34 on systems where it is not needed, declare the dependency with environment markers. Installing certain versions of enum34 on systems where it is not needed at all, might result in compile time issues. This is the recommended approach discussed here:
python-poetry/poetry#1122

Modifications

  • setup.py: use environment markers for enum34 instead of custom logic that is not preserved in pypi metadata for the wheel

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as all python tests

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): no
  • The public API: no
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no
  • The rest endpoints: no
  • The admin cli options: no
  • Anything that affects deployment: don't know

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable
  • If a feature is not applicable for documentation, explain why? Only packaging change

To prevent installing enum34 on systems where it is not needed, declare the dependency with environment markers. Installing certain versions of enum34 on systems where it is not needed at all, might result in compile time issues. This is the recommended approach discussed here:
python-poetry/poetry#1122
@languitar
Copy link
Contributor Author

Can't see how the failing test is related to this pull request

@tuteng
Copy link
Member

tuteng commented Oct 10, 2020

/pulsarbot run-failure-checks

@rosejn
Copy link

rosejn commented Oct 12, 2020

This enum34 dependency is causing major issues for our projects, and this environment marker would fix that. I see people have been having issues with enum34 for over a year now. Can we not just get rid of it, or at minimum only use it as a dependency when using an old python version? I'm happy to help get this moving in any way possible, as we have already lost many man hours dealing with the fallout.

@sijie
Copy link
Member

sijie commented Oct 13, 2020

@rosejn If you have a better solution than this pull request, feel free to submit one. So the community can take a look at the change and merge it if it solves the problem.

@languitar
Copy link
Contributor Author

@rosejn If you have a better solution than this pull request, feel free to submit one. So the community can take a look at the change and merge it if it solves the problem.

Is there anything wrong with this PR that would require a different solution? According to @rosejn the marker that I introduced would solve the problem.

@sijie
Copy link
Member

sijie commented Oct 13, 2020

@languitar I don't think so. I wasn't sure what does his comment mean. So I was asking if he is thinking about a different solution than this PR.

@rosejn
Copy link

rosejn commented Oct 13, 2020

Hi guys, after my team just waisted way too much time battling with this issue I was frustrated and trying to encourage forward motion on this PR, so my comment wasn't very clear. If there was something holding this back I wanted to volunteer to do whatever was needed to help get a fix merged.

@sijie sijie merged commit edf0944 into apache:master Oct 13, 2020
@rosejn
Copy link

rosejn commented Oct 13, 2020

Thanks guys! I'm happy to test this out to double check that things work as hoped for. I've just pulled the latest on master and run the docker-build.sh script in pulsar-client-cpp, where it successfully built. Anyone know how to then try this out as a poetry dependency?

wolfstudy pushed a commit that referenced this pull request Oct 30, 2020
Fixes #7050 

### Motivation

To prevent installing enum34 on systems where it is not needed, declare the dependency with environment markers. Installing certain versions of enum34 on systems where it is not needed at all, might result in compile time issues. This is the recommended approach discussed here:
python-poetry/poetry#1122

### Modifications

* `setup.py`: use environment markers for enum34 instead of custom logic that is not preserved in pypi metadata for the wheel

(cherry picked from commit edf0944)
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Nov 13, 2020
Fixes apache#7050 

### Motivation

To prevent installing enum34 on systems where it is not needed, declare the dependency with environment markers. Installing certain versions of enum34 on systems where it is not needed at all, might result in compile time issues. This is the recommended approach discussed here:
python-poetry/poetry#1122

### Modifications

* `setup.py`: use environment markers for enum34 instead of custom logic that is not preserved in pypi metadata for the wheel
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[python] pulsar-client@2.5.2's dep on enum34@1.1.9 may break build systems
5 participants