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

Fix WebIO's compat for JSExpr #7145

Closed
wants to merge 1 commit into from
Closed

Conversation

twavv
Copy link
Contributor

@twavv twavv commented Dec 25, 2019

I'm not 100% on the version specification format here. My understanding is that 0.5 means [0.5.0, 0.6).

Please merge ASAP if looks good. :^)

I'm not 100% on the version specification format here. My understanding is that `0.5` means `[0.5.0, 0.6)`.

Please merge ASAP if looks good. :^)
@twavv
Copy link
Contributor Author

twavv commented Dec 25, 2019

To register WebIO v0.8.13 (lucky number 13!), I added JSExpr to the compat because I was trying to fix an issue where JSExpr v1 has just been released and was incompatible.

JSExpr v0.5 depends on WebIO, but in v1.0, we dropped the dependency (and were planning on reversing it for WebIO v1.0). I forgot that WebIO doesn't actually depend on JSExpr though when I added the compat entry.

Is there any way to resolve this issue? Does Pkg allow circular dependencies? :^) Another "solution" might be to un-release JSExpr v1.0 since it was only really released in preparation for WebIO v1.0 (these issues would still exist but they wouldn't be as bad because someone installing JSExpr and WebIO from scratch would get the latest versions that work with each other).

@DilumAluthge
Copy link
Member

Does Pkg allow circular dependencies?

No.

@DilumAluthge
Copy link
Member

What are you trying to accomplish here?

@DilumAluthge DilumAluthge added do not merge help wanted Extra attention is needed labels Dec 25, 2019
@twavv
Copy link
Contributor Author

twavv commented Dec 25, 2019

JSExpr depended upon WebIO but that was kind of bad™ so we removed the dependency in v1 and were going to have WebIO depend on JSExpr to implement some webio-specific functionality in WebIO v1 (but that's a WIP).

As it stands, because we reversed the direction of the dependency, we can't constraint what version of JSExpr gets installed.

@DilumAluthge
Copy link
Member

This can’t be merged in its current state because it is causing CI to fail.

I’m not sure this is the correct way to fix this situation.

Perhaps one of @fredrikekre @KristofferC @StefanKarpinski can help out with this after the holidays.

@twavv
Copy link
Contributor Author

twavv commented Dec 25, 2019

This can’t be merged in its current state because it is causing CI to fail.

Yeah, I made this PR thinking that WebIO depended on JSExpr and then figuring out that we're in the current situation. :^)

One solution would be to allow "peer dependency" specifications (à la "A doesn't depend on B, but if B is installed it must be version v1.2.3") but that's a structural change to the Pkg system and I have a feeling that it wouldn't be well received by most (for probably pretty valid reasons).

I suspect that the immediate fix would be to yank JSExpr v1 for now and un-yank them once we we have WebIO v1.0 ready to go. There would still be some of these issues but at least ]add WebIO JSExpr would install compatible packages.

@DilumAluthge
Copy link
Member

If the issue here is that you made a bad release of JSExpr, then you should yank that bad release.

@DilumAluthge
Copy link
Member

Just be aware that you cannot “unyank” a release. So if you released 1.0 and now you want to yank it, you can yank 1.0, but you can’t later unyank it. You can however make a new release, e.g. 1.1 or 2.0.

@DilumAluthge
Copy link
Member

I think the correct solution here is for you to yank JSExpr 1.0. And then, when you are ready, you can release either JSExpr 1.1 or 2.0.

The solution definitely isn’t to add all of these dependencies retroactively, so I’m closing this PR.

@DilumAluthge
Copy link
Member

#7165

@DilumAluthge DilumAluthge removed compat fix help wanted Extra attention is needed labels Dec 26, 2019
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.

None yet

2 participants