Project

General

Profile

Actions

action #110142

closed

Sync tools/tidy and .perltidyrc

Added by tinita almost 2 years ago. Updated over 1 year ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Feature requests
Target version:
Start date:
2022-04-20
Due date:
2022-05-05
% Done:

0%

Estimated time:

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.

Actions #1

Updated by openqa_review almost 2 years ago

  • Due date set to 2022-05-05

Setting due date based on mean cycle time of SUSE QE Tools

Actions #3

Updated by tinita almost 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.

Actions #4

Updated by tinita almost 2 years ago

  • Status changed from In Progress to Feedback
Actions #5

Updated by tinita almost 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

Actions #6

Updated by pvorel over 1 year 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?

Actions #7

Updated by tinita over 1 year 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.

Actions #8

Updated by pvorel over 1 year 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)

Actions #9

Updated by tinita over 1 year 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.

Actions

Also available in: Atom PDF