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

SLICOT 5.8.0, Julia 1.8 compat #5259

Merged
merged 4 commits into from Sep 18, 2022
Merged

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented Aug 1, 2022

Someone should do something like #4770 (for lack of better options at the moment)

Originally posted by @giordano in #4969 (comment)

I'm "someone".

https://discourse.julialang.org/t/too-many-julia-versions/84538/15?u=mkitti

  1. Do we need a version bump to SLICOT 5.8.1 to do this?
  2. @andreasvarga can we coordinate a SLICOT 5.8.1 release?

cc: @RalphAS

> Someone should do something like JuliaPackaging#4770 (for lack of better options at the moment)

I'm "someone".

https://discourse.julialang.org/t/too-many-julia-versions/84538/15?u=mkitti

1. Do we need a version bump to SLICOT 5.8.1 to do this?
2. @andreasvarga can we coordinate a SLICOT 5.8.1 release?

cc: @RalphAS
@mkitti mkitti changed the title SLICOT 5.8, Julia 1.8 compat SLICOT 5.8.1, Julia 1.8 compat Aug 1, 2022
@andreasvarga
Copy link

andreasvarga commented Aug 1, 2022

OK. So once again my reply to your proposal.

The best for SLICOT integration would be of course to have compatibility starting with the LTS version, as well as with the current and future versions. I appreciate very much your help in this respect. If necessary, I could produce a version bump to 5.8.1 for SLICOT itself, just to keep the right correspondence between the versions. Is this OK for you?

@mkitti
Copy link
Contributor Author

mkitti commented Aug 1, 2022

I'm not sure if compatibility with Julia 1.6 is possible at this point since I think the version mechanism depends on new functionality, but I would be happy to corrected.

@andreasvarga
Copy link

This was only a wishful thinking of me (not important for further developments).

@RalphAS
Copy link
Contributor

RalphAS commented Aug 1, 2022

Thanks for picking this up. You might have a new record for the number of tarballs.

Julia v1.6 would need a different approach because libblastrampoline was only partly integrated. It could be difficult to keep the versioning in order with the two builds in parallel.

@RalphAS
Copy link
Contributor

RalphAS commented Aug 2, 2022

Now I've remembered why I didn't do this earlier: the builds should really be sorted by version of libblastrampoline rather than version of Julia, and I didn't figure out a simple way to do that.

@andreasvarga
Copy link

Sorry for my ignorance, but what follows now? In the meantime, I have troubles even registering under Julia v1.7 (see #68456).

@giordano
Copy link
Member

This is failing to build. If someone could debug this, that'd be much appreciated.

@RalphAS
Copy link
Contributor

RalphAS commented Sep 18, 2022

The linker is failing to find libblastrampoline. It seems to be looking in /workspace/destdir/lib which it gets as the shell variable ${libdir}. Has there been a relevant recent change in BinaryBuilder?

@giordano
Copy link
Member

No

@mkitti
Copy link
Contributor Author

mkitti commented Sep 18, 2022

It is only failing to find libblastrampoline on certain platform triples.

@mkitti
Copy link
Contributor Author

mkitti commented Sep 18, 2022

The Julia 1.7 builds succeed. Thr Julia 1.8 and Julia 1.9 builds fail.

@andreasvarga
Copy link

For Julia 1.7 v3.0.4 of blastrampoline is used, which is OK. For Julia 1.8 and 1.9, version v5.1.0 is used and not the last available version v5.1.1. I wonder if this is a problem. Moreover, for 1.8 and 1.9, version v3.0.4 is also mentioned in the script. I can not figure out what effect this may have.

@mkitti
Copy link
Contributor Author

mkitti commented Sep 18, 2022

OK, I will try that.

Also do you have any plans to release a SLICOT 5.8.1. This will be released as 5.8.1 and it would be nice to keep the versions synchronized.

@andreasvarga
Copy link

I observed that the last call to generate the binaries contains a dependency to julia 1.7. Is this a lower limit on the version or it must be also adapted to various julia versions ?

@andreasvarga
Copy link

I propose to keep the version of SLICOT as it is now.

@mkitti
Copy link
Contributor Author

mkitti commented Sep 18, 2022

At the moment, I believe we still need to bump the version number here in this packaging for Julia 1.7 compat at least.

@mkitti
Copy link
Contributor Author

mkitti commented Sep 18, 2022

I observed that the last call to generate the binaries contains a dependency to julia 1.7. Is this a lower limit on the version or it must be also adapted to various julia versions ?

julia_compat=v"1.7" indicates compatibility with Julia versions [1.7, 2.0) per https://pkgdocs.julialang.org/v1/compatibility/#Version-specifier-format

@giordano
Copy link
Member

I'm not entirely sure we need to bump the version number: that's necessary when changing the generated project file (new dependencies, or julia compat), which isn't the case here? There was a libblastrampoline compat, but that was kinda redundant since libblastrampoline is a stdlib, bound to julia anyway.

@mkitti
Copy link
Contributor Author

mkitti commented Sep 18, 2022

b9a67f0 looks good for 1.8 compat.

  • Expanding again to 1.7 and 1.9
  • Shoved this package version back to 5.8.0

Let's see if this still works.

@mkitti
Copy link
Contributor Author

mkitti commented Sep 18, 2022

CI is green. Version is 5.8.0. Let's go?

@@ -103,7 +119,8 @@ products = [

# Dependencies that must be installed before this package can be built
dependencies = [
Dependency(PackageSpec(name="libblastrampoline_jll", uuid="8e850b90-86db-534c-a0d3-1478176c7d93"); compat="3.0.4"),
Dependency(get_addable_spec("libblastrampoline_jll", v"3.0.4+0"); platforms=filter(p -> VersionNumber(p["julia_version"]) == v"1.7.0", platforms)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, this line doesn't seem right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's fine? What values does p["julia_version"] cover?

Copy link
Member

Choose a reason for hiding this comment

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

julia_versions = [v"1.7.0", v"1.8.0", v"1.9.0"]

@giordano giordano changed the title SLICOT 5.8.1, Julia 1.8 compat SLICOT 5.8.0, Julia 1.8 compat Sep 18, 2022
@giordano giordano merged commit e9e42e3 into JuliaPackaging:master Sep 18, 2022
@andreasvarga
Copy link

Congratulations and many thanks!

@RalphAS
Copy link
Contributor

RalphAS commented Sep 19, 2022

Don't celebrate too soon; the package doesn't seem to be consistent yet: JuliaRegistries/General#68512
says

ERROR: Unsatisfiable requirements detected for package libblastrampoline_jll [8e850b90]:
libblastrampoline_jll [8e850b90] log:
├─possible versions are: 5.1.1 or uninstalled
└─found to have no compatible versions left with SLICOT_jll [545525a2]
└─SLICOT_jll [545525a2] log:
├─possible versions are: 5.7.0-5.8.0 or uninstalled
└─restricted to versions 5.8.0 by an explicit requirement, leaving only versions 5.8.0

Previously, @giordano fixed a similar problem by adding a compat bound for LBT to Project.toml, but that won't work when the compat depends on the Julia version. Maybe drop Julia v1.7 and set the LBT bound to 5.0?

@mkitti
Copy link
Contributor Author

mkitti commented Sep 19, 2022

Actually, I'm pretty sure the current issue is due to the previous compat bound for libblastrampoline_jll.

https://github.com/JuliaRegistries/General/pull/68512/files does not attempt to change Compat.toml. So the existing Compat.toml still applies:
https://github.com/JuliaRegistries/General/blob/master/S/SLICOT_jll/Compat.toml

@andreasvarga
Copy link

andreasvarga commented Sep 19, 2022

This is a similar problem for which I opened an issue when trying to register a new version of PeriodicSystems. This issue has been closed, with the conclusion that

AutoMerge always runs with the latest stable Julia, which is currently Julia 1.8. If your package does not support Julia 1.8, it will fail AutoMerge, and you will need to request a manual merge.

However, in our case we are at a lower level (registering the library itself), so I assume if we manage to solve this issue we will solve also #68456 .

Just a remark: The PR example used as template for producing SLICOT_jll was for a package/library Pfapack, which apparently was never registered in Julia (only a Python version exists)!

@mkitti
Copy link
Contributor Author

mkitti commented Sep 19, 2022

JuliaRegistries/General#68512 has been merged.

Now we need to turn our attention to PeriodicSystems we have compat issues over there now.

@mkitti
Copy link
Contributor Author

mkitti commented Sep 19, 2022

https://juliahub.com/ui/Packages/Pfapack_jll/HOmkl/0.1.0+1?page=1 Pfapack_jll is registered by the way.

@andreasvarga
Copy link

I formulated an issue regarding the failure of PeriodicSystems release 0.4.2 on Windows with Julia 1.8 (with Julia 1.7 it works, as well as on Linux). I can easily reproduce the failure locally by using any call involving the wrappers I implemented for several SLICOT routines. Each call crashes Julia (so I can not see the actual error), but from the CI.log, the error is basically

Exception: EXCEPTION_ACCESS_VIOLATION at 0x0 -- unknown function (ip: 0000000000000000)
in expression starting at D:\a\PeriodicSystems.jl\PeriodicSystems.jl\test\test_pschur.jl:15
unknown function (ip: 0000000000000000)
Allocations: 107208134 (Pool: 107190352; Big: 17782); GC: 43

I guess, something is wrong with the generated SLICOT_jll for Windows under Julia 1.8.

Very gratefull for any idea how to fix this issue.

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.

None yet

4 participants