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

fix invalid escape sequences in easyconfigs by using raw strings (r"...") #11149

Merged
merged 6 commits into from
Feb 11, 2023

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Aug 20, 2020

Similar to #11148 this handles the invalid escape sequences by using raw strings for all ECs

I.e. it fixes the <string>:25: DeprecationWarning: invalid escape sequence \$ warnings when parsing an EC and the related flake8 warning

This is clean with flake8 --select W605 --filename '*.eb' -j5 now

Copy link
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

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

I think that this change will cause issues with quote_str(), which can escape backslashes. Raw strings will end up having double backslashes.

@lexming lexming added the change label Aug 25, 2020
@lexming lexming added this to the 4.x milestone Aug 25, 2020
@Flamefire
Copy link
Contributor Author

If that is the case maybe the function needs a fix? I'm not sure how it is used though so can't tell if it causes problems
If so then better use escaped backslash

@Flamefire
Copy link
Contributor Author

Quick ping here. Still getting those DeprecationWarnings and I think we should fix those before they become errors. And the problem just get worse as more and more ECs get added by copying those "mistakes"

@boegel boegel modified the milestones: 4.x, next release (4.5.0?) Sep 17, 2021
@boegel boegel modified the milestones: 4.5.0 (next release), 4.x Oct 28, 2021
@Flamefire Flamefire force-pushed the flake8_ecs_string branch 2 times, most recently from d40cbe3 to 94913e9 Compare July 26, 2022 09:12
@Flamefire
Copy link
Contributor Author

Rebased again. What is the status here @boegel ?

Copy link
Member

@jfgrimm jfgrimm left a comment

Choose a reason for hiding this comment

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

As decided in issue #16330, we have deprecated the use of True to signify a system-toolchain dependency (#16384), in favour of the more intuitive SYSTEM template constant. Due to the change in the test suite, please run eb --sync-pr-with-develop 11149 and update the PR to use SYSTEM instead.

@Flamefire
Copy link
Contributor Author

@jfgrimm I'm not sure your comment applies here as this is only a refactoring to fix the style/warnings not changing or introducing anything

Anyway can this and #11148 be reviewed and merged please so we can enable flake8 on CI and avoid reintroducing the issues I already fixed?

I just rebased this, resolved the conflict and checked that flake8 --select W605 --filename '*.eb' -j5 doesn't return anything

@Flamefire
Copy link
Contributor Author

I extracted the shovill changes into a separate PR and added a CI check to avoid reintroducing this. That can be extended once the other flake8-related PRs have been merged.

@lexming
Copy link
Contributor

lexming commented Feb 11, 2023

Test report by @lexming
SUCCESS
Build succeeded for 58 out of 58 (58 easyconfigs in total)
node300.hydra.os - Linux CentOS Linux 7.9.2009, x86_64, Intel(R) Xeon(R) Gold 6148 CPU @ 2.40GHz, Python 3.6.8
See https://gist.github.com/lexming/203867d0566011912c771172b4ebea10 for a full test report.
note: only a single easyconfig per package tested and only on recent toolchains

Copy link
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

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

LGTM

@lexming
Copy link
Contributor

lexming commented Feb 11, 2023

Going in, thanks @Flamefire !

@lexming lexming merged commit ab65047 into easybuilders:develop Feb 11, 2023
@lexming lexming added the tests label Feb 11, 2023
@lexming
Copy link
Contributor

lexming commented Feb 11, 2023

Tests rolled-back to normal state in #17304

@Flamefire Flamefire deleted the flake8_ecs_string branch February 11, 2023 15:11
@boegel boegel changed the title Fix invalid escape sequences in ECs fix invalid escape sequences in easyconfigs by using raw strings (r"...") Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants