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

Back out one of the changes in #26178 to get warnings/warnings.chpl working #26180

Merged

Conversation

bradcray
Copy link
Member

While paratesting my branch tonight, I found that

chplenv/chplconfig/warnings/warnings.chpl

was failing due to PR #26178. Though I'm not that familiar with this logic, it seems that reverting half of the changes in it keeps this test working as expected, though it may suggest we need to do more to both achieve the PR's aim and also keep errors for cases like this.

…ings.chpl working

While paratesting my branch tonight, I found that

  chplenv/chplconfig/warnings/warnings.chpl

was failing due to PR chapel-lang#26178.  Though I'm not that familiar with this
logic, it seems that reverting half of the changes in it keeps this
test working as expected, though it may suggest we need to do more to
both achieve the PR's aim and also keep errors for cases like this.

---
Signed-off-by: Brad Chamberlain <[email protected]>
@bradcray
Copy link
Member Author

@jabraham17: I'll need a post-review merge on this, and for you to check how much of your fix this undoes / whether we can have both the warning and support the case you were striving for.

@DanilaFe
Copy link
Contributor

The warning in question relied (prior to Jade's recent PR) on the fact that splitting a list with more than two elements as a, b = l throws an error. But with maxsplit=1, the list will always have two elements, so the error won't be thrown in that case, and the format warning won't be emitted.

There's a tug between allowing = on the right side of VAR=... and ruling out cases such as A==B, which can be parsed as A = (=B). I think it's reasonable to add a special-case warning for == specifically -- two consecutive equal signs, which would fix this test. In the meantime, reverting the change for the night should avoid nightly testing calamities.

@bradcray bradcray merged commit 56a218a into chapel-lang:main Oct 31, 2024
7 checks passed
@bradcray bradcray deleted the fix-warnings-test-partially-back-out-26178 branch October 31, 2024 05:35
@jabraham17
Copy link
Member

I agree with everything Daniel said. This change looks good and I will do a proper fix this morning that allows VAR= ...=... but not VAR==

Thank you both for avoiding a slew of nightly failures this morning :)

jabraham17 added a commit that referenced this pull request Oct 31, 2024
Properly handles the parsing of chplconfig files.
#26178 added the ability to
parse chplconfig variables with values including `=`, but that broke the
warning for something like `CHPL_VAR==value`.

This PR resolves the issue by refactoring some of the parsing code in
`overrides.py` to check error cases in `skip_line`, rather than relying
on python exceptions. Then to avoid duplication of logic, renames
`skip_line` to `check_line` and has it return the parsed variable.

Fixes the issue introduced in
#26178 and reverts
#26180

Tested by running `start_test test/chplenv`

[Reviewed by @DanilaFe]
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.

3 participants