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

Set proper lower bound for portalocker dependency, drop packaging dependency #125

Merged
merged 1 commit into from
Dec 27, 2023

Conversation

akx
Copy link
Contributor

@akx akx commented Dec 21, 2023

packaging was only used as a dependency to check for the version of portalocker in use.

This drops that dependency and instead corrects the minimum version of portalocker to the one checked here.

It should be safe to depend on a version of portalocker that has been out for 3 years at this point, and is not quite as intrusive as #117 that would remove portalocker altogether (and in all honesty, that PR has been out there for a year, with no sign of merging).

@akx
Copy link
Contributor Author

akx commented Dec 21, 2023

@microsoft-github-policy-service agree

@rayluo
Copy link
Contributor

rayluo commented Dec 22, 2023

There is a history behind it.

packaging was only used as a dependency to check for the version of portalocker in use.

This drops that dependency and instead corrects the minimum version of portalocker to the one checked here.

It should be safe to depend on a version of portalocker that has been out for 3 years at this point

We started with portalocker=~1.6 in the first place, and then @bluca convinced us to relax the lower bound for Linux. If @bluca can chime in to say it is now OK to also use portalocker 1.6+ on Linux, we will consider this PR.

#117 that would remove portalocker altogether (and in all honesty, that PR has been out there for a year, with no sign of merging).

You can chime in in that PR and ping the reviewer there. That PR was developed based on a need at that time, but the integration test effort was suspended, so, there is little motivation to move forward.

@akx
Copy link
Contributor Author

akx commented Dec 22, 2023

@rayluo Thanks for the feedback. My angle to this is trying to prune extra dependencies that aren't really needed – seems a waste to depend on (and download!) all of packaging just to be able to compare a version to another.

If, AIUI, not upgrading Portalocker is mostly the issue of more work for downstream Linux packagers, I understand and we'll need to deal accordingly, packagers can be an ornery bunch 😄

  • It looks like Ubuntu 20.04 (still LTS supported) has python3-portalocker 1.5.1-1, so that would be an issue for pinning to 1.6+, but not to pinning to 1.4+ (which adds file_open_kwargs).
  • Debian Bullseye (the oldest still supported Debian) has 2.2.1, not an issue.

(I don't quite know how to check for the other major distros.)

Maybe the heuristic for whether the file_open_kwargs can be passed could be done without an external dependency – some ideas:

  • parse portalocker.__version__ "by hand" into a version_info style tuple and compare it
  • check whether file_open_kwargs exists in the __doc__ for portalocker.Lock.__init__ – a bit hacky, maybe, but should do the trick?

@akx
Copy link
Contributor Author

akx commented Dec 22, 2023

IOW, @bluca, what do you think, would Portalocker >= 1.5 be a good middle ground 3 years after #49? 😁

@bluca
Copy link
Contributor

bluca commented Dec 22, 2023

IOW, @bluca, what do you think, would Portalocker >= 1.5 be a good middle ground 3 years after #49? 😁

yes, fine by me

@akx
Copy link
Contributor Author

akx commented Dec 22, 2023

@rayluo Okay, rebased + pushed a version that changes the pin to >= 1.4.

@rayluo rayluo added the enhancement New feature or request label Dec 26, 2023
@rayluo rayluo merged commit a10d092 into AzureAD:dev Dec 27, 2023
20 checks passed
@akx akx deleted the drop-packaging-dep branch December 27, 2023 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants