Add support for RPM transaction scripts

[jtyr]

The RPM vfs is missing support transaction scripts.

[jtyr]

0001-Adding-support-for-RPM-transaction-scripts.patch

[zaytsev]

I would love to see this tested, but I'm not sure of how to hook the tester in. The way list function is constructed in this extfs helper is a bit peculiar :-/

(See README in tests/src/vfs/extfs/helpers-list.)

I'm leaning towards committing as is :-(

[mooffie]

Replying to zaytsev:

I would love to see this tested, but I'm not sure of how to hook the tester in. The way list function is constructed in this extfs helper is a bit peculiar :-/

I gave that helper only a relatively-quick view but I suspect I know what you mean:

1. The helper needs (I think) more than the one command (MC_TEST_EXTFS_LIST_CMD) the tester gives it. Fortunately, additional commands can be easily synthesized. I wrote a ​draft explaining it here.

2. Another potential problem is calling the external program this way (just an example):

$RPM --list-contents a b c
$RPM --get-tags a b c

This should be changed to:

$RPM_LIST a b c
$RPM_GET_TAGS a b c

(This way we can override $RPM_LIST and $RPM_GET_TAGS separately.)

[jtyr]

I will have a look into how to improve this.

[mooffie]

Replying to jtyr:

I will have a look into how to improve this.

Just a sec! I didn't mean to say that a test should be written as precondition for committing your patch (I was about to clarify this, but you were quicker than me).

I think that if your patch looks safe enough then the maintainers should go ahead and commit it.

Of course, if you want to help us by trying to write a test then this would be greatly appreciated.

[zaytsev]

@mooffie, you are right, but what I deem to be even more annoying is that rpm is triggered many times via mcrpmfs_printOneMetaInfo with different parameters and returns varying output according to the arguments.

It's not like you run the archiver once and parse its output, or something, such that the output can be simply replaced with cat'ing from the mock data file. Currently, I don't see a better solution other than mocking rpm -qp --qf from mcrpmfs_getRawOneTag with a hashtable, which is doable, but very irritating (and on top of that one would need to mock mcrpmfs_getAllNeededTags as well).

[zaytsev]

I don't know if I'm being too softball to accept patches without tests, even for code that never had tests in the first place. Many projects I'm working with just flat out refuse to look into stuff if it isn't accompanied with tests, and I totally sympathize with that. However, it's difficult to be a hardliner in a situation where maintainers are severely underpowered and the coverage in general is a disaster... :-/

[jtyr]

Another thing is that the current version of the tester doesn't allow to test the content of the files created in the vfs. That's quite important for the RPM helper.

[zaytsev]

Another thing is that the current version of the tester doesn't allow to test the content of the files created in the vfs. That's quite important for the RPM helper.

You are right about that, but already being able to test the list function is God send. Most extfs helpers work by parsing the output from external tools to generate listings, like various archivers, and it's breaking all the time due to changes in the archivers, mc, non-robust parser code, bug fixes that introduce new bugs, etc.

[jtyr]

0002-Removing-PROG-files-and-improving-code-structure.patch

[mooffie]

I'm studying the rpm helper now. I think I'm about to change my mind: testing it may not be hard. Stay tuned.

@jtyr: You're mixing formatting with code-modifications in your patch. That's a big no-no. We won't be able to see what you've changed. Besides, you'd better hold off your work a bit till I study the testability issue.

[mooffie]

(Of course, if #3751 is rejected this one gets unblocked.)

[mooffie]

Ok, we're ready to roll.

[mooffie]

I've created a branch for @jtyr's 1st patch:

branch: 3750_extfs_rpm_trans_scripts
Initial changeset:4310b066e23b1e88256ab9beaeb06711a144147b

It contains two commits: one to implement the feature, one to "fix" the now outdated test.

(@Yury: Is it ok to separate the two? This won't cause trouble to git bisect when both the break and the fix are inside a merged branch, right?)

[mooffie]

Andrew / Yury:

@jtyr's 2nd patch implements another feature (see next comment) but it's far from ready, so I suggest we deal with it separately (maybe in another ticket; we don't even know @jtyr will return to address the issues).

So, vote for the branch as-is. I'm adding my vote.

[mooffie]

@jtyr:

As for your 2nd patch:

It's a mixed bag. It's not easy to review it because the actual changes are buried among "formatting". So we can't accept this patch as-is.

The patch gets rid of INFO/SCRIPT/$(tag)PROG in favor of shebang lines in the corresponding INFO/SCRIPT/$(tag) files. This in itself sounds fine (I'm not an rpm user, though). A shebang line has limitations (max two tokens), but perhaps you intend these lines to be informative only. If you're still interested in this, please create a patch dealing with this alone. Maybe we'd better do this in a separate ticket.

BTW, instead of doing code formatting you can improve the code. I.e., instead of:

INFO/SCRIPTS/PREIN) 
    echo -n "#!" >"$2" 
    mcrpmfs_getRawOneTag "%{RPMTAG_PREINPROG}\n" >>"$2" 
    mcrpmfs_getRawOneTag "%{RPMTAG_PREIN}\n" >>"$2" 
    exit 0 
    ;; 
INFO/SCRIPTS/POSTIN) 
    echo -n "#!" >"$2" 
    mcrpmfs_getRawOneTag "%{RPMTAG_POSTINPROG}\n" >>"$2" 
    mcrpmfs_getRawOneTag "%{RPMTAG_POSTIN}\n" >>"$2" 
    exit 0 
    ;; 
... repeating ...

maybe do something along of:

INFO/SCRIPTS/{PREIN,POSTIN,...}) 
    write_script_file "$1" "$2"

[jtyr]

I'm fine if you merge only the first patch for now and I will bring the additional changes in a separate ticket later on.

[zaytsev]

(@Yury: Is it ok to separate the two? This won't cause trouble to git bisect when both the break and the fix are inside a merged branch, right?)

That's a very controversial topic; tldr: in my personal opinion it's still worth separating the fix & the test, even if it makes bisecting a bit more difficult.

The reason for it is that I like it when while fixing a regression one commits the test first to show the reviewer / let the CI check that the test actually fails with the regression present! How many tests I have seen that claimed to test for a regression, but actually didn't :-/ because they were just wishful thinking added past fixing the regression... or else when implementing a new feature, it's nice when the tests are adjusted separately from the code implementing the changes. Whether they should come first or last is a philosophical question; as I favor TDD when appropriate, I tend to commit the tests first, but I don't get upset when one checks in some new stuff, and then adjusts the tests, in the end it amounts to the same. It's only regression tests that I'd really like to see first.

Now, as you said, this doesn't make git bisect all too happy if as your criterion you just let all of the tests run, instead of picking a particular one, because bisection is not branch-aware.

However, if one does have a relatively clean history (as we are trying to keep), one can always bisect at merge commit level and then when a particular feature branch has been identified, go one level deeper with a trick like:

# bisect feature branches
git bisect start master good

for rev in $(git rev-list good..master --merges --first-parent); do
    git rev-list $rev^2 --not $rev^
done | xargs git bisect skip

git bisect run

# go deeper
...

I also remember some talk about adding --first-parent to git bisect, but I don't think it actually materialized in the end. And, of course, none of that matters if your test is specific enough...

[zaytsev]

It's a mixed bag. It's not easy to review it because the actual changes are buried among "formatting". So we can't accept this patch as-is.

Just a tip that I often use to clean up whitespace soup contributions: apply the patch and then regenerate it by running

git diff -b [ or even -w ] --ignore-blank-lines

Now, you can reapply whitespace fixes separately if/when needed. Have to be careful though... but better than nothing.

[mooffie]

Replying to jtyr:

I'm fine if you merge only the first patch for now and I will bring the additional changes in a separate ticket later on.

Great! I'll be closing this ticket then.

Replying to zaytsev:

[...] as I favor TDD when appropriate, I tend to commit the tests first, but I don't get upset when [...] It's only regression tests that I'd really like to see first. [...] one can always bisect at merge commit level [...]

Interesting! Thank you for all this info.

[mooffie]

Merged to master: [43d7fce730ae2cbf58a47a035f1ed5a050dc1610]

[ossi]

Replying to zaytsev:

(@Yury: Is it ok to separate the two? This won't cause trouble to git bisect when both the break and the fix are inside a merged branch, right?)

That's a very controversial topic; tldr: in my personal opinion it's still worth separating the fix & the test, even if it makes bisecting a bit more difficult.

this is a false dichotomy. the canonical way is adding the test first, but marking it as an expected failure (XFAIL).

[zaytsev]

... only, unfortunately, many harnesses, including the custom one in question here, do not support marking tests as expected failures.

[ossi]

i suspected that this might be the case. it would still the right thing to do, and should be acknowledged as the first option. one can still debate the pros and cons of suboptimal approaches after establishing that the preferable one is not feasible.

[mooffie]

Replying to ossi:

the canonical way is adding the test first, but marking it as an expected failure (XFAIL).

Good idea! I'll bear that in mind.

Replying to zaytsev:

... unfortunately [the tester does] not support marking tests as expected failures.

I've learned quite a bit since then (and still do). This will be addressed sometime soon I hope. (I'm currently preoccupied with some other corner of MC, which is why you don't hear from me.)