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

Don't refer to IterativeEigensolvers or SuiteSparse in Compat.jl #578

Merged
merged 1 commit into from
Jun 18, 2018

Conversation

andreasnoack
Copy link
Member

We are in the process of converting these two stdlibs to normal packages. However, Compat causes the PR to fail because Documenter depends on Compat. Hence, this PR removes any mentioning of these two stdlibs.

@martinholters
Copy link
Member

Should we keep the functionality on older Julia versions in order not to break anything? We did so in similar cases.

@andreasnoack
Copy link
Member Author

andreasnoack commented Jun 17, 2018

I don't think we should spend cycles on avoiding breakage between prerelease versions.

Copy link
Member

@ararslan ararslan left a comment

Choose a reason for hiding this comment

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

Looks good to me, but it might be worth a note in the README that these were removed, since there are a few packages currently using Compat.IterativeEigensolvers and whatnot, and the package authors may come here looking for answers as to why those modules suddenly disappeared.

@andreasnoack
Copy link
Member Author

Do you have an idea about how many packages we are talking about?

@ararslan
Copy link
Member

Not offhand, it's more an anecdotal observation than hard data. Maybe this is just fine as-is and if someone is confused in an issue, on Discourse, or wherever then we can add a note later.

@martinholters
Copy link
Member

I don't think we should spend cycles on avoiding breakage between prerelease versions.

My main concern here would be packages targeting 0.6 which have already updated to be compatible with 0.7-alpha and then suddenly break/need to put an upper bound on the Compat version.

@andreasnoack
Copy link
Member Author

My main concern here would be packages targeting 0.6 which have already updated to be compatible with 0.7-alpha and then suddenly break/need to put an upper bound on the Compat version.

That is true. It wouldn't just be a matter of breaking code people experimenting with the alpha. It would break code for people using 0.6. I'm not sure how you'd manage your proposal though. Should Compat start depending on Arpack in order to be able to load eigs? It seems problematic. Ideally, I can find all the package that uses IterativeEigensolvers and warn them.

@martinholters
Copy link
Member

martinholters commented Jun 18, 2018

No, I'd just make the current code conditional on a version where we get IterativeEigensolvers without mentioning it in Project.toml, and stop mentioning it in the README. Then it's not usable for compatibility between 0.6 and 0.7, but at least it won't break any code on 0.6. That would be in the same spirit as #558 and #559, and so far, nobody has come complaining 😄

@andreasnoack
Copy link
Member Author

I've updated the PR. I believe this version should avoid breaking anything on 0.6 and older versions of 0.7-.

Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

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

LGTM.

@andreasnoack andreasnoack merged commit 3cba0d5 into master Jun 18, 2018
@andreasnoack andreasnoack deleted the anj/stdlib branch June 18, 2018 12:01
@dsweber2
Copy link

Not sure if I should make a separate issue, but I'm getting breaking behavior in 0.7 after adding Compat, which seems to be related to this issue. The error is

ERROR: LoadError: ArgumentError: Package Compat does not have IterativeEigensolvers in its dependencies:
- If you have Compat checked out for development and have
  added IterativeEigensolvers as a dependency but haven't updated your primary
  environment's manifest file, try `Pkg.resolve()`.
- Otherwise you may need to report an issue with Compat
Stacktrace:
 [1] require(::Module, ::Symbol) at ./loading.jl:830
 [2] top-level scope at /home/dsweber/.julia/packages/Compat/Hrh9l/src/Compat.jl:921
 [3] include at ./boot.jl:317 [inlined]
 [4] include_relative(::Module, ::String) at ./loading.jl:1038
 [5] include(::Module, ::String) at ./sysimg.jl:29
 [6] top-level scope at none:2
 [7] eval at ./boot.jl:319 [inlined]
 [8] eval(::Expr) at ./client.jl:399
 [9] top-level scope at ./none:3
in expression starting at /home/dsweber/.julia/packages/Compat/Hrh9l/src/Compat.jl:910
ERROR: Failed to precompile Compat [34da2185-b29b-5c13-b0c7-acf172513d20] to /home/dsweber/.julia/compiled/v0.7/Compat/GSFWK.ji.
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] macro expansion at ./logging.jl:313 [inlined]
 [3] compilecache(::Base.PkgId, ::String) at ./loading.jl:1185
 [4] _require(::Base.PkgId) at ./logging.jl:311
 [5] require(::Base.PkgId) at ./loading.jl:852
 [6] macro expansion at ./logging.jl:311 [inlined]
 [7] require(::Module, ::Symbol) at ./loading.jl:834

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

Successfully merging this pull request may close these issues.

4 participants