Skip to content

Revert "Enable JuliaSyntax.jl as the default Julia parser" #50227

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.

Sign up for GitHub

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

Jump to bottom
Closed
DilumAluthge wants to merge 1 commit into from
Closed

Revert "Enable JuliaSyntax.jl as the default Julia parser" #50227

DilumAluthge wants to merge 1 commit into from
+270 −425

Conversation

@DilumAluthge
Copy link
Member

@DilumAluthge DilumAluthge commented Jun 20, 2023

Reverts #46372

It looks like that PR broke the build job for 32-bit Windows.

@DilumAluthge DilumAluthge added the revert This reverts a previously merged PR. label Jun 20, 2023
@DilumAluthge DilumAluthge requested a review from giordano June 20, 2023 13:59
@DilumAluthge
Copy link
Member Author

DilumAluthge commented Jun 20, 2023

cc: @oscardssmith @c42f

@oscardssmith
Copy link
Member

oscardssmith commented Jun 20, 2023

See #50205. It appears to be unrelated.

@gbaraldi
Copy link
Member

gbaraldi commented Jun 20, 2023

I think this PR pushed it through the brink, but it has been broken on the cusp of breaking for a while.

giordano
giordano approved these changes Jun 20, 2023
View reviewed changes
Copy link
Member

@giordano giordano left a comment

Choose a reason for hiding this comment

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

I don't think having CI systematically broken is acceptable. As Gabriel said, it has been close to breaking for a while, but now CI is finally red everywhere.

The underlying problem has to be addressed as quickly as possible, but it shouldn't be used as excuse for having useless CI.

@c42f
Copy link
Member

c42f commented Jun 20, 2023

I don't think we should revert this wholesale. Better to add a build-time option to not have JuilaSyntax as the default parser in the sysimage?

@c42f
Copy link
Member

c42f commented Jun 20, 2023

It'll take a little investigation to add a build-time option but hopefully I can have a PR for that in fairly short order.

@giordano
Copy link
Member

giordano commented Jun 20, 2023

I think build problems have been mitigated by JuliaCI/julia-buildkite#308?

@c42f
Copy link
Member

c42f commented Jun 20, 2023

Ok excellent. So I don't need to rush forward making a quick fix at this point?

@giordano
Copy link
Member

giordano commented Jun 20, 2023

I don't think so. Also, it was a more fundamental problem than avoiding building JuliaSyntax, another large change would have pushed beyond the limit anyway, so an ad-hoc solution for a single PR wouldn't be sufficient.

I think also #50237 is meant to further reduce memory usage in general.

@pchintalapudi pchintalapudi deleted the revert-46372-cjf/juliasyntax-stdlib branch June 21, 2023 04:21
@pablosanjose
Copy link
Contributor

pablosanjose commented Jun 21, 2023

Not sure if it is the same as for 32bit windows, but I can no longer build the mac app from source (contrib/mac/app), apparently due to a JuliaSyntax issue. It fails with the following error

...
    CC usr/tools/stringreplace
Building HTML documentation.
  Installing known registries into `~/Build/julia/doc/deps`
┌ Warning: The active manifest file has dependencies that were resolved with a different julia version (1.9.0-DEV). Unexpected behavior may occur.
└ @ ~/Build/julia/doc/Manifest.toml:0
   Installed Parsers ───────────── v2.4.0
   Installed IOCapture ─────────── v0.2.2
   Installed JSON ──────────────── v0.21.3
   Installed DocStringExtensions ─ v0.9.1
   Installed ANSIColoredPrinters ─ v0.0.1
   Installed Documenter ────────── v0.27.23
Precompiling project...
  7 dependencies successfully precompiled in 5 seconds
  1 dependency had warnings during precompilation:
┌ Parsers [69de0a69-1ddd-5017-9359-2bf0b02dc9f0]
│  WARNING: method definition for dpeekbyte at /Users/pablo/Build/julia/doc/deps/packages/Parsers/34hDN/src/utils.jl:199 declares type variable T but does not use it.
└
[ Info: SetupBuildDirectory: setting up build directory.
┌ Error: JuliaSyntax parser failed — falling back to flisp!
│   exception =
│    BoundsError: attempt to access empty SubString{String} at index [0]
│    Stacktrace:
│      [1] checkbounds(s::SubString{String}, I::Int64)
│        @ Base ./strings/basic.jl:216 [inlined]
│      [2] getindex(s::SubString{String}, i::Int64)
│        @ Base ./strings/substring.jl:91
│      [3] getindex(source::Base.JuliaSyntax.SourceFile, i::Int64)
│        @ Base.JuliaSyntax ~/Build/julia/base/JuliaSyntax/src/source_files.jl:119
│      [4] highlight(io::IOBuffer, source::Base.JuliaSyntax.SourceFile, range::UnitRange{…}; color::Tuple{…}, context_lines_before::Int64, context_lines_inner::Int64, context_lines_after::Int64, note::String, notecolor::Symbol)
│        @ Base.JuliaSyntax ~/Build/julia/base/JuliaSyntax/src/source_files.jl:221
│      [5] show_diagnostic(io::IOBuffer, diagnostic::Base.JuliaSyntax.Diagnostic, source::Base.JuliaSyntax.SourceFile)
│        @ Base.JuliaSyntax ~/Build/julia/base/JuliaSyntax/src/diagnostics.jl:76
│      [6] show_diagnostics(io::IOBuffer, diagnostics::Vector{Base.JuliaSyntax.Diagnostic}, source::Base.JuliaSyntax.SourceFile)
│        @ Base.JuliaSyntax ~/Build/julia/base/JuliaSyntax/src/diagnostics.jl:86

@giordano
Copy link
Member

giordano commented Jun 21, 2023

No. You want to open a new issue for that.

@pablosanjose
Copy link
Contributor

pablosanjose commented Jun 21, 2023

Done, #50245

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@giordano giordano giordano approved these changes

Assignees

No one assigned

Labels

revert This reverts a previously merged PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@DilumAluthge @oscardssmith @gbaraldi @c42f @giordano @pablosanjose