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

why is the compat section of this package so restrictive? #58

Closed
ExpandingMan opened this issue Nov 4, 2019 · 15 comments
Closed

why is the compat section of this package so restrictive? #58

ExpandingMan opened this issue Nov 4, 2019 · 15 comments
Assignees

Comments

@ExpandingMan
Copy link
Contributor

ExpandingMan commented Nov 4, 2019

The compat section of the Project.toml for this package is very restrictive, and is wreaking havoc on my environment. Are there specific reasons for all these restrictions (the reason for my skepticism is that this package is largely a simple HTTP wrapper)? If not, I'd beg that we can remove them. Unnecessary compat restrictions are really harmful as they cause packages to be downgraded unnecessarily, if more packages were so restrictive, there'd be a high risk of getting unsatisfiable requirements.

@mattBrzezinski mattBrzezinski self-assigned this Nov 4, 2019
@mattBrzezinski
Copy link
Member

I added dependency versions in the compat section as the JuliaRegistrator zeptodoctor bot requires them for automerge:

JuliaRegistries/General#4846 (comment)

The versions in the Project.toml were the only ones I was able to get the project building with successfully. If this is a blocking issue, and problematic for lots of people I'm fine with removing the changes and having JuliaRegistrator be a manual process.

Do others have any thoughts?

@ExpandingMan
Copy link
Contributor Author

The versions in the Project.toml were the only ones I was able to get the project building with successfully.

I've just tried building it with the compat section removed, and it definitely builds. The question is, do tests pass? (unfortunately it's very hard to test this package). Again, my skepticism here comes from the nature of this package. I could definitely at least imagine there being cases in which one needed to restrict AWSCore, HTTP or EzXML, but there is an awful lot of stuff being restricted here by this very, very minimal package. I haven't been able to completely disentagle why it downgrades so much for me.

@mattBrzezinski
Copy link
Member

I've just tried building it with the compat section removed, and it definitely builds. The question is, do tests pass? (unfortunately it's very hard to test this package).

I don't see why they wouldn't unless there's an underlying issue with a dependency. Before the changes in #56 I didn't run into any issues with testing.

Again, my skepticism here comes from the nature of this package. I could definitely at least imagine there being cases in which one needed to restrict AWSCore, HTTP or EzXML, but there is an awful lot of stuff being restricted here by this very, very minimal package.

It seems that zeptodoctor looks at non-base deps and forces a restriction on them in the compat section for auto-merge.

@omus @iamed2 @rofinn any thoughts on this?

@KristofferC
Copy link

The compat section in this file is following best practices and is not overly restrictive.

@oxinabox
Copy link
Contributor

oxinabox commented Nov 4, 2019

I was talking to @mattBrzezinski when he made the PR to add Compat versions.
He even went back, and carefully tested each version of each package that has multiple versions listed.
To make sure they worked.

He did the good thing.
Rather than just test on the latest version of everything that he was using and locking compat to that.

If there is a particular package compat restriction you would like to see relaxed.
Then open an issue called:
"Can the restriction on HTTP be relaxed to 0.7,0.8"
Or what ever the restriction is that is hurting.

The overall practice here is the correct one.

@ExpandingMan
Copy link
Contributor Author

I'm not accusing anyone of anything wrong here, but for a package this simple to cause so many downgrades and nobody knows exactly why does not bode well at all for a future in which all packages have these restrictions. When I have time I will dig through this and figure out what the problem is.

@mattBrzezinski
Copy link
Member

but for a package this simple to cause so many downgrades and nobody knows exactly why

Could you post the packages and their versions in your environment? I can help try and root cause the issue in some free time.

does not bode well at all for a future in which all packages have these restrictions

It's definitely possible to end up in this state, and I can see the concern. I don't think this is the package to have this discussion in, but I'd raise your concerns with it in either the Julia Slack / Discourse / Git Issue page(s).

@ExpandingMan
Copy link
Contributor Author

Ok, I've looked into this a bit more. It turns out that the most egregious problems were not caused by this package (I still don't know where they are coming from), but one thing from this package that is likely causing issues is the bound on MbedTLS. This package had an 0.7 release way back in June, is there some reason it is capped to 0.6 in AWSS3.jl?

@mattBrzezinski
Copy link
Member

mattBrzezinski commented Nov 5, 2019

Ok, I've looked into this a bit more. It turns out that the most egregious problems were not caused by this package (I still don't know where they are coming from), but one thing from this package that is likely causing issues is the bound on MbedTLS. This package had an 0.7 release way back in June, is there some reason it is capped to 0.6 in AWSS3.jl?

AWSCore.jl currently restricts [email protected], which prevents this package from upgrading to [email protected].

https://github.com/JuliaCloud/AWSCore.jl/blob/v0.6.4/Project.toml#L26

@oxinabox
Copy link
Contributor

oxinabox commented Nov 5, 2019

AFAICT this package doesn't actually directly load MbedTLS.jl ?
So we can probably just drop it from the dependencies entirely?

MbedTLS.jl releases tend to need to be synconized against Julia releases for reasons I don't understand entirely,b but that i think boil down to Julia also including the MbedTLS shared library and that gets mad if you link against common versions. So I bet that once can be relaxed a bunch?

@mattBrzezinski
Copy link
Member

AFAICT this package doesn't actually directly load MbedTLS.jl ?
So we can probably just drop it from the dependencies entirely?

MbedTLS is being used here:

https://github.com/JuliaCloud/AWSS3.jl/blob/master/src/AWSS3.jl#L790

@mattBrzezinski
Copy link
Member

@mattBrzezinski
Copy link
Member

AWSCore.jl has a CR going through for including [email protected], link.

I'll create a CR to bump the version here as well. However it should be noted that if you're using [email protected], you will be locked to Julia v1.2.X.

@oxinabox
Copy link
Contributor

oxinabox commented Nov 7, 2019

Why does this load MbedTLS in the middle of the file?

using MbedTLS

:-(

@omus
Copy link
Member

omus commented Feb 24, 2020

I'll close this issue as I'm pretty certain there isn't anything actionable left here. As of AWSCore v0.6.5 there is now support for MbedTLS v0.7 (requires Julia 1.2). That change should take care of the downgrades that were observed.

@omus omus closed this as completed Feb 24, 2020
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

No branches or pull requests

5 participants