action #110142
closedSync tools/tidy and .perltidyrc
Description
Both scripts in os-autoinst and openQA have diverged.
Also configs have diverged.
This is annoying when moving code between repos and suddenly style checks fail.
I already started with this because this started with a comparably small task of adding OpenQA::Test::PatchDeparse to os-autoinst-common, when we found that both perltidy configs are different.
I also remembered again that the tools/tidy scripts are different and I wanted to sync those a long time ago.
Updated by openqa_review over 2 years ago
- Due date set to 2022-05-05
Setting due date based on mean cycle time of SUSE QE Tools
Updated by tinita over 2 years ago
Updated by tinita over 2 years ago
Like mentioned in the pull requests:
For now I'm not adding this to os-autoinst-common because I have to solve the problem that os-autoinst-distri-opensuse uses readlink -f $0 to get the os-autoinst/cpanfile.
But as discussed in today's daily, it might be even better if os-autoinst-distri-opensuse would use their own cpanfile for specifying the Perl::Tidy version, so that a change in our os-autoinst doesn't require immediate action in the other repository.
Then all 3 repos could use tools/tidy from os-autoinst-common.
Updated by tinita over 2 years ago
- Status changed from In Progress to Feedback
Still waiting for feedback on https://github.com/os-autoinst/openQA/pull/4613
Updated by tinita over 2 years ago
- Status changed from Feedback to Resolved
https://github.com/os-autoinst/openQA/pull/4613 merged
Not syncing .perltidyrc because of too many differences in os-autoinst-distri-opensuse
Updated by pvorel about 2 years ago
tinita wrote:
Not syncing .perltidyrc because of too many differences in os-autoinst-distri-opensuse
How about sync tools/tidy, but keep .perltidyrc repo specific?
Updated by tinita about 2 years ago
pvorel wrote:
tinita wrote:
Not syncing .perltidyrc because of too many differences in os-autoinst-distri-opensuse
How about sync tools/tidy, but keep .perltidyrc repo specific?
Sure, but see comment #3 about the problem with that.
Updated by pvorel about 2 years ago
tinita wrote:
How about sync tools/tidy, but keep .perltidyrc repo specific?
Sure, but see comment #3 about the problem with that.
For now I'm not adding this to os-autoinst-common because I have to solve the problem that os-autoinst-distri-opensuse > > uses readlink -f $0 to get the os-autoinst/cpanfile.
But as discussed in today's daily, it might be even better if os-autoinst-distri-opensuse would use their own cpanfile > for specifying the Perl::Tidy version, so that a change in our os-autoinst doesn't require immediate action in the other > repository.
You mean in tools/tidy:
dir="$(dirname "$0")"
...
# This might be used from another repo like os-autoinst-distri-opensuse
if [ -z "${perltidy_version_expected}" ]; then
# No cpanfile in the linked repo, use the one from os-autoinst instead
dir="$(dirname "$(readlink -f "$0")")"
I'm adding full path without reading symlink in https://github.com/os-autoinst/os-autoinst/pull/2154
dir="$(realpath -s $(dirname "$0"))"
If we accept that PR, wouldn't it solve just symlink cpanfile from os-autoinst? I'll put that note to the PR.
edited by tinita: add code markers (three backticks)
Updated by tinita about 2 years ago
In that case the whole if block would be useless, as it would be checking the same cpanfile again.
Currently it checks os-autoinst-distri-opensuse/cpanfile, and if there is no Perl::Tidy entry in there, it goes into the if block and checks os-autoinst-distri-openuse/os-autoinst/cpanfile (via readlink).
With your change it would check os-autoinst-distri-opensuse/cpanfile twice.
os-autoinst-distri-opensuse/tools/tidy is a symlink to os-autoinst-distri-opensuse/os-autoinst/tools/tidy.
It uses readlink to find the cpanfile from os-autoinst: os-autoinst-distri-opensuse/os-autoinst/cpanfile in order to grep for the Perl::Tidy version in that file.
If we moved/symlinked os-autoinst-distri-opensuse/tools/tidy to os-autoinst-common/tools/tidy, then thanks to the readlink it would search for a os-autoinst-common/cpanfile, but there is none.
That's why I suggested in comment #3 that os-autoinst-distri-opensuse/cpanfile gets its own entry for Perl::Tidy.
If we can agree on that, then we can remove the whole if block.
The only downside is that the Perl::Tidy version needs to be manually adjusted in os-autoinst-distri-opensuse/cpanfile from time to time.