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

Update and load styledstrings #51869

Merged
merged 2 commits into from
Feb 1, 2024

Conversation

tecosaur
Copy link
Contributor

This has some value by itself, but mainly serves as a prerequisite for #51811, #51816, and #51829, along with two other PRs that have yet to be made.

@tecosaur tecosaur added REPL Julia's REPL (Read Eval Print Loop) stdlib Julia's standard library don't squash Don't squash merge labels Oct 25, 2023
@tecosaur tecosaur force-pushed the update-and-load-styledstrings branch from 8f7b078 to 2de89d9 Compare October 25, 2023 16:16
@LilithHafner
Copy link
Member

CI fails because of JuliaLang/StyledStrings.jl#8

@LilithHafner
Copy link
Member

Triage is happy with letting REPL depend StyledStrings.jl

@tecosaur
Copy link
Contributor Author

tecosaur commented Nov 9, 2023

Ok, the problem seems to be this:

Pkg                                               (3) |         failed at 2023-11-08T23:46:57.570
Error During Test at /cache/build/tester-amdci4-9/julialang/julia-master/julia-db6dc6445f/share/julia/stdlib/v1.11/Pkg/test/new.jl:3007
  Got exception outside of a @test
  Unsatisfiable requirements detected for package StyledStrings [f489334b]:
   StyledStrings [f489334b] log:
   ├─StyledStrings [f489334b] has no known versions!
   └─found to have no compatible versions left with REPL [3fa0cd96]
     └─REPL [3fa0cd96] log:
       ├─possible versions are: 1.11.0 or uninstalled

I only find this confusing though, if someone could weigh in that would be much appreciated 🙏.

@vtjnash
Copy link
Member

vtjnash commented Nov 10, 2023

I think Pkg has some special internal list of stdlibs, as otherwise it needs to go find them in General, and I don't think it seems to be registered there

@KristofferC
Copy link
Member

I think Pkg has some special internal list of stdlibs, as otherwise it needs to go find them in General, and I don't think it seems to be registered there

Hm, Pkg looks these up at runtime by looking in the stdlib folder so it should find them. I'll try figure out what is going on..

@tecosaur
Copy link
Contributor Author

Thanks Kristoffer :)

@KristofferC
Copy link
Member

Okay, the tests that error is within the "historical stdlib" stuff that I do not really know much about. Maybe @staticfloat knows what needs to be done here. It is possible it needs to be added to https://github.com/JuliaPackaging/HistoricalStdlibVersions.jl/blob/3eccb253cd154d56c0efd75d2315b459725addf9/src/version_map.jl#L1515 (which might be what Jameson meant) .

@staticfloat
Copy link
Member

I pushed a new commit to that repo; it's supposed to have a bot that auto-opens new PRs I think, but it doesn't seem to be working.

@tecosaur
Copy link
Contributor Author

tecosaur commented Dec 4, 2023

To get this moving, @staticfloat might it be worth manually opening a PR?

@tecosaur tecosaur force-pushed the update-and-load-styledstrings branch from db6dc64 to 8d848dd Compare December 28, 2023 03:38
@tecosaur
Copy link
Contributor Author

I've tried updating the branch in the hope that the "historical stdlib" issue has been fixed in the background. I guess I'll find out if this is the case or not from CI.

@tecosaur
Copy link
Contributor Author

Still seems like the same issue is occurring

Pkg                                               (2) |         failed at 2023-12-28T04:20:31.766
Error During Test at /cache/build/tester-amdci4-8/julialang/julia-master/julia-8d848dde20/share/julia/stdlib/v1.11/Pkg/test/new.jl:3027
  Got exception outside of a @test
  Unsatisfiable requirements detected for package StyledStrings [f489334b]:
   StyledStrings [f489334b] log:
   ├─StyledStrings [f489334b] has no known versions!
   └─found to have no compatible versions left with REPL [3fa0cd96]
     └─REPL [3fa0cd96] log:
       ├─possible versions are: 1.11.0 or uninstalled

@tecosaur
Copy link
Contributor Author

I can't see how I can solve this myself, so if @KristofferC or @staticfloat could spare this some more attention, that would be much appreciated.

@KristofferC
Copy link
Member

JuliaPackaging/HistoricalStdlibVersions.jl#17 pushed a new commit but a new version for this was never registered. I will try bump the version and register it and we can rerun CI here and see what happens.

@tecosaur
Copy link
Contributor Author

Thanks, Kristoffer. I just took a look at the CI for the commit you pushed and I see the same error (StyledStrings [f489334b] has no known versions!) in the buildkite/julia-master task. Am I correct in guessing that you pushed that commit after the new version was registered?

@tecosaur
Copy link
Contributor Author

If I could give this a gentle bump once more, with the PRs blocked by this in particular it would be very good if this could be worked out.

@tecosaur
Copy link
Contributor Author

@KristofferC might you be able to spare the time to give this another look?

@tecosaur tecosaur force-pushed the update-and-load-styledstrings branch 2 times, most recently from f6aa299 to e57e3b4 Compare January 31, 2024 09:30
@vchuravy
Copy link
Member

So the version with StyledStrings was registered JuliaRegistries/General#97838

Is Pkg using the right version of HistoricalStdlibVersions?

@KristofferC
Copy link
Member

It should install it here https://github.com/JuliaLang/Pkg.jl/blob/b13bd2ddf6049865ddd8f7984bc9aa2790c14d21/test/runtests.jl#L42-L47.

But I don't see anything about it getting installed in the log.

@KristofferC
Copy link
Member

KristofferC commented Jan 31, 2024

I confirm this is an issue even with the latest HistoricalStdlibVersions.

However, I think this is a bug with the implementation in Pkg of the whole julia_version thing. In order to properly implement this feature we need to know the dependencies of the stdlibs for every Julia version, not only what packages are stdlibs. What happens now is that we call Pkg.add(...; julia_version=v"1.5") which will read in the stdlibs from v1.5 but the dependencies of (in this case) REPL is still read from its project file which includes StyledStrings which was not an stdlib in v1.5.

I would suggest disabling this Pkg test for now since it is unfortunate to block this on a Pkg bug.

Edit: JuliaLang/Pkg.jl#3773 for the test disabling

@tecosaur
Copy link
Contributor Author

tecosaur commented Feb 1, 2024

Now that #53138 is merged, this should be mergeable without any CI issues 🙂

@tecosaur
Copy link
Contributor Author

tecosaur commented Feb 1, 2024

CI hasn't quite finished yet, but so far I'm seeing:

  • A Downloads error
  • More Downloads errors
  • A strings/basic error that will need to be fixed in StyledStrings

The strings/basic error

julia> Base.return_types(write, (IO, AbstractString))
5-element Vector{Any}:
 Int64
 Int64
 Int64
 Int64
 Int64

julia> using StyledStrings

julia> Base.return_types(write, (IO, AbstractString))
6-element Vector{Any}:
 Int64
 Int64
 Int64
 Any
 Int64
 Int64

I'll have a look at the write methods it defines and push a fix shortly.

This gets us two particular commits of interest:
- Replace within-module eval with hygienic eval: which makes it possible
  to include StyledStrings in the sysimage without running into
  precompile errors.
- Load the JULIA_*_COLOR env vars for compat: which mirrors the current
  behaviour to the relevant faces.
By loading the StyledStrings stdlib in REPL, we load the privateered
print/show methods for the Annotated{String,Char} types defined there.

This is nice to have, because it means that styled annotated strings can
be constructed in Base and elsewhere without loading the StyledStrings
stdlib, but they will be displayed as intended in the REPL.
@tecosaur tecosaur force-pushed the update-and-load-styledstrings branch from 07e1629 to 30ccace Compare February 1, 2024 12:54
@tecosaur
Copy link
Contributor Author

tecosaur commented Feb 1, 2024

Ok, at this point it seems like all the CI has run other than the Adress Sanitised/gnuassert builds.

I'm not seeing any non-spurious errors other than a Error: Error setting up bootstrap issue with the Mingw32 builds, which I'm pretty sure has nothing to do with this PR.

@tecosaur
Copy link
Contributor Author

tecosaur commented Feb 1, 2024

Alrighty, all the tests have finished now, and it looks all clear to me! The only non-allowed fail is the Address Sanitised build timing out.

@vtjnash vtjnash merged commit 6e7db14 into JuliaLang:master Feb 1, 2024
5 of 7 checks passed
@tecosaur
Copy link
Contributor Author

tecosaur commented Feb 1, 2024

I'm so glad to see this merged, thanks to those who helped work out what was going on here and fix it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
don't squash Don't squash merge REPL Julia's REPL (Read Eval Print Loop) stdlib Julia's standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants