-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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 #3967 Merge of Prediction Context arrays was broken #4180
Conversation
…omparing two pointers! Fixes: antlr#3967 Closes antlr#3967 - A small error was made whereby the comparison with a newly created merge `M` of two prediction contexts `a` and `b`, was comparing the pointers of `a` and `M` and not using the internal Equals() method. The ATN output trace is now equal. - Also adds a new testrig internal project (so it won't be used by `go get`) that allows quick setup and testing of ATN tracing for a test grammar and test input. - Excludes go performance profiles from git - Corrects a small error in one of the ATN tracing scripts. Signed-off-by: Jim.Idle <jimi@idle.ws>
… fixed Signed-off-by: Jim.Idle <jimi@idle.ws>
@parrt Usual CPP tests failing - who can we get to fix this? |
@parrt I am backed up on this PR, if you have a chance to merge it? Cheers |
No idea how to fix those C++ build errors. some github actions crap. :( @ericvergnaud have any ideas? |
@hs-apotell is looking into it |
I was wondering what we are actually doing in that action. Is there an
actual need to upload the C++ output artifacts? As the Tommy Cooper joke
goes:
Doctor, doctor, I’ve broken my arm in three places.
Don’t go to those three places again!
Maybe we just stop uploading and let everyone build the target runtimes
theirselves? I realize that this comment should be on the thread about this
by @hs-apotell.
…On Sun, Mar 19, 2023 at 03:54 ericvergnaud ***@***.***> wrote:
@hs-apotell <https://github.com/hs-apotell> is looking into it
—
Reply to this email directly, view it on GitHub
<#4180 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ7TMFQTZSYCTNGXMKCJQTW4YHFRANCNFSM6AAAAAAV3KUA34>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Not all users have the development environment setup to build and debug every platform that antlr supports. The artifacts are uploaded to ease that pain. |
Hmm. I hear what you’re saying, but if users are using the C++ runtime,
they will surely have the ability to build it? Do we know if anyone is
actually using these artifacts?
…On Sun, Mar 19, 2023 at 14:25 HS ***@***.***> wrote:
Not all users have the development environment setup to build and debug
every platform that antlr supports. The artifacts are uploaded to ease that
pain.
—
Reply to this email directly, view it on GitHub
<#4180 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ7TMBCOTXARDZ3RDUHPLTW42RGTANCNFSM6AAAAAAV3KUA34>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
The issue is not restricted to C++ specific CI jobs. They are, however, prone to this error more than others. Don't ask why because I don't have a convincing answer for that. But going thru' the past history of builds on CI, you can notice the error across all different flavors. Size of the upload may be a criteria. But again, that hasn't really changed much and yet the issue started happening a lot more often only recently. Anyone using the build - yes, I do occasionally but not the cpp ones but other including csharp, dart, go and other mac specific targets. They have been useful in resolving issues that I can't reproduce locally or I don't have an environment to test for (like mac). Regardless, the change I introduced here in this PR #4186 is a compromise solution. An attempt is made to upload the artifacts but a failure wouldn't make the entire build fail. If need be, the user can trigger a rebuild of specific target to get the artifact if the previous attempt failed. |
Ok that’s cool. If it isn’t stopping the build anymore, then I guess it
doesn’t matter, but I am skeptical that storing the build outputs is
useful. If you don’t have a Mac, then I can’t see the value of being able
to download it. But seeing that it builds and tests correctly is necessary
of course.
I don’t have a windows machine anymore so I don’t have any use for
downloading the C++ library built for windows. Maybe I am missing something
here?
There’s no artifact for Go, as the source code is what the user downloads.
But as I said, if it doesn’t stop the build, then no harm no foul.
…On Sun, Mar 19, 2023 at 16:48 HS ***@***.***> wrote:
The issue is not restricted to C++ specific CI jobs. They are, however,
prone to this error more than others. Don't ask why because I don't have a
convincing answer for that. But going thru' the past history of builds on
CI, you can notice the error across all different flavors. Size of the
upload may be a criteria. But again, that hasn't really changed much and
yet the issue started happening a lot more often only recently.
Anyone using the build - yes, I do occasionally but not the cpp ones but
other including csharp, dart, go and other mac specific targets. They have
been useful in resolving issues that I can't reproduce locally or I don't
have an environment to test for (like mac).
Regardless, the change I introduced here in this PR #4186
<#4186> is a compromise solution. An
attempt is made to upload the artifacts but a failure wouldn't make the
entire build fail. If need be, the user can trigger a rebuild of specific
target to get the artifact if the previous attempt failed.
—
Reply to this email directly, view it on GitHub
<#4180 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ7TMFSNPNDNWAQCOUXB3TW43B7FANCNFSM6AAAAAAV3KUA34>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
The reason the build outputs/artifacts are useful is because the logs from the builds aren't readily available for diagnosis from the CI window. Either someone can spend the time to cherry-pick the logs for each configuration/platform/target and upload them as artifact or bundle up the whole thing and throw it up there. I chose the latter solution to cover the ground for all platforms and targets (and partially because I don't develop for all the supported platforms/targets). Can the artifact size be optimized? Yes, of course. But they aren't large enough to be a problem. Other projects I work on each artifact is over 1GB and each build can generate anywhere from 2 to 8 such artifacts. So, compared to those artifacts, antlr generated artifacts are minuscule in size. |
fix: Fixes merge arrays to perform an actual comparison rather than comparing two pointers!
Fixes: #3967
Closes #3967
A small error was made whereby the comparison with a newly created merge
M
of two prediction contextsa
andb
, was comparing the pointers ofa
andM
and not using the internalEquals()
method.The ATN output trace is now equal.
Also adds a new
testrig
internal project (so it won't be used bygo get
) that allows quick setupand testing of ATN tracing for a test grammar and test input.
Excludes go performance profiles from git
Corrects a small error in one of the ATN tracing scripts.