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.

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

Change check for embedded llvm version number to a regex to make test more flexible. #79528

Merged
merged 1 commit into from
Jan 26, 2024
Merged

Change check for embedded llvm version number to a regex to make test more flexible. #79528

merged 1 commit into from
Jan 26, 2024

Conversation

dyung
Copy link
Collaborator

@dyung dyung commented Jan 26, 2024

This test started to fail when LLVM created the release/18.x branch and the main branch subsequently had the version number increased from 18 to 19.

I investigated this failure (it was blocking our internal automation) and discovered that the CHECK statement on line 27 seemed to have the compiler version number (1800) encoded in octal that it was checking for. I don't know if this is something that explicitly needs to be checked, so I am leaving it in, but it should be more flexible so the test doesn't fail anytime the version number is changed. To accomplish that, I changed the check for the 4-digit version number to be a regex.

I originally updated this test for the 18->19 transition in a01195f. This change makes the CHECK line more flexible so it doesn't need to be continually updated.

@dyung dyung requested a review from ysyeda January 26, 2024 00:32
@dyung dyung mentioned this pull request Jan 26, 2024
Merged
@tstellar
Copy link
Collaborator

It would probably be better just to remove this check. I don't know why we would need to check the compiler version in the test.

@dyung
Copy link
Collaborator Author

dyung commented Jan 26, 2024

It would probably be better just to remove this check. I don't know why we would need to check the compiler version in the test.

I agree, but if we do need to keep the check, we should at least make it a regex. Or if the version should match something else, then the test should be rewritten to do that.

tstellar
tstellar approved these changes Jan 26, 2024
View reviewed changes
Copy link
Collaborator

@tstellar tstellar left a comment

Choose a reason for hiding this comment

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

Let's merge this now, because this test is failing on the release branch and blocking the release. We can always clean up the test and remove the version check in a later commit.

ysyeda
ysyeda approved these changes Jan 26, 2024
View reviewed changes
Copy link
Contributor

@ysyeda ysyeda left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@dyung dyung merged commit 45f883e into llvm:main Jan 26, 2024
3 of 4 checks passed
@dyung
Copy link
Collaborator Author

dyung commented Jan 26, 2024

@tstellar what is the process for getting commits included on the release branch? Should I open another PR with this change against the release/18.x branch?

@tstellar
Copy link
Collaborator

@tstellar what is the process for getting commits included on the release branch? Should I open another PR with this change against the release/18.x branch?

Yes, you can do that.

dyung added a commit to dyung/llvm-project that referenced this pull request Jan 26, 2024
@dyung
Change check for embedded llvm version number to a regex to make test…
d787fef 
… more flexible. (llvm#79528)

This test started to fail when LLVM created the release/18.x branch and
the main branch subsequently had the version number increased from 18 to
19.

I investigated this failure (it was blocking our internal automation)
and discovered that the CHECK statement on line 27 seemed to have the
compiler version number (1800) encoded in octal that it was checking
for. I don't know if this is something that explicitly needs to be
checked, so I am leaving it in, but it should be more flexible so the
test doesn't fail anytime the version number is changed. To accomplish
that, I changed the check for the 4-digit version number to be a regex.

I originally updated this test for the 18->19 transition in
a01195f. This change makes the CHECK
line more flexible so it doesn't need to be continually updated.

(cherry picked from commit 45f883e)
tstellar pushed a commit that referenced this pull request Jan 27, 2024
@dyung
Change check for embedded llvm version number to a regex to make test…
147c623 
… more flexible. (#79528) (#79642)

This test started to fail when LLVM created the release/18.x branch and
the main branch subsequently had the version number increased from 18 to
19.

I investigated this failure (it was blocking our internal automation)
and discovered that the CHECK statement on line 27 seemed to have the
compiler version number (1800) encoded in octal that it was checking
for. I don't know if this is something that explicitly needs to be
checked, so I am leaving it in, but it should be more flexible so the
test doesn't fail anytime the version number is changed. To accomplish
that, I changed the check for the 4-digit version number to be a regex.

I originally updated this test for the 18->19 transition in
a01195f. This change makes the CHECK
line more flexible so it doesn't need to be continually updated.

(cherry picked from commit 45f883e)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
@dyung @tstellar
Change check for embedded llvm version number to a regex to make test…
e220e14 
… more flexible. (llvm#79528) (llvm#79642)

This test started to fail when LLVM created the release/18.x branch and
the main branch subsequently had the version number increased from 18 to
19.

I investigated this failure (it was blocking our internal automation)
and discovered that the CHECK statement on line 27 seemed to have the
compiler version number (1800) encoded in octal that it was checking
for. I don't know if this is something that explicitly needs to be
checked, so I am leaving it in, but it should be more flexible so the
test doesn't fail anytime the version number is changed. To accomplish
that, I changed the check for the 4-digit version number to be a regex.

I originally updated this test for the 18->19 transition in
a01195f. This change makes the CHECK
line more flexible so it doesn't need to be continually updated.

(cherry picked from commit 45f883e)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
@dyung @tstellar
Change check for embedded llvm version number to a regex to make test…
01e4856 
… more flexible. (llvm#79528) (llvm#79642)

This test started to fail when LLVM created the release/18.x branch and
the main branch subsequently had the version number increased from 18 to
19.

I investigated this failure (it was blocking our internal automation)
and discovered that the CHECK statement on line 27 seemed to have the
compiler version number (1800) encoded in octal that it was checking
for. I don't know if this is something that explicitly needs to be
checked, so I am leaving it in, but it should be more flexible so the
test doesn't fail anytime the version number is changed. To accomplish
that, I changed the check for the 4-digit version number to be a regex.

I originally updated this test for the 18->19 transition in
a01195f. This change makes the CHECK
line more flexible so it doesn't need to be continually updated.

(cherry picked from commit 45f883e)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
@dyung @tstellar
Change check for embedded llvm version number to a regex to make test…
bce9c04 
… more flexible. (llvm#79528) (llvm#79642)

This test started to fail when LLVM created the release/18.x branch and
the main branch subsequently had the version number increased from 18 to
19.

I investigated this failure (it was blocking our internal automation)
and discovered that the CHECK statement on line 27 seemed to have the
compiler version number (1800) encoded in octal that it was checking
for. I don't know if this is something that explicitly needs to be
checked, so I am leaving it in, but it should be more flexible so the
test doesn't fail anytime the version number is changed. To accomplish
that, I changed the check for the 4-digit version number to be a regex.

I originally updated this test for the 18->19 transition in
a01195f. This change makes the CHECK
line more flexible so it doesn't need to be continually updated.

(cherry picked from commit 45f883e)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
@dyung @tstellar
Change check for embedded llvm version number to a regex to make test…
78a0a47 
… more flexible. (llvm#79528) (llvm#79642)

This test started to fail when LLVM created the release/18.x branch and
the main branch subsequently had the version number increased from 18 to
19.

I investigated this failure (it was blocking our internal automation)
and discovered that the CHECK statement on line 27 seemed to have the
compiler version number (1800) encoded in octal that it was checking
for. I don't know if this is something that explicitly needs to be
checked, so I am leaving it in, but it should be more flexible so the
test doesn't fail anytime the version number is changed. To accomplish
that, I changed the check for the 4-digit version number to be a regex.

I originally updated this test for the 18->19 transition in
a01195f. This change makes the CHECK
line more flexible so it doesn't need to be continually updated.

(cherry picked from commit 45f883e)
@pointhex pointhex mentioned this pull request May 7, 2024
Closed
@aemerson aemerson mentioned this pull request May 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tstellar tstellar tstellar approved these changes

@ysyeda ysyeda ysyeda approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dyung @tstellar @ysyeda