Changed by jtyr (jiri.tyr@….com) on Dec 29, 2016 at 21:15 UTC

• Attachment 0001-Adding-support-for-RPM-transaction-scripts.patch (raw) added

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

Changed by zaytsev (@zyv) on Dec 30, 2016 at 6:30 UTC (comment 1)

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 :-(

Changed by mooffie (@mooffie) on Dec 30, 2016 at 13:40 UTC (comment 1.2)

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.)

Changed by jtyr (jiri.tyr@….com) on Dec 30, 2016 at 13:51 UTC (comment 3)

I will have a look into how to improve this.

Changed by mooffie (@mooffie) on Dec 30, 2016 at 13:57 UTC (comment 3.4)

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.

Changed by zaytsev (@zyv) on Dec 30, 2016 at 14:03 UTC (comment 5)

@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).

Changed by zaytsev (@zyv) on Dec 30, 2016 at 14:08 UTC (comment 6)

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... :-/

Changed by jtyr (jiri.tyr@….com) on Dec 30, 2016 at 14:56 UTC (comment 7)

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.

Changed by zaytsev (@zyv) on Dec 30, 2016 at 15:59 UTC (comment 8)

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.

Changed by jtyr (jiri.tyr@….com) on Dec 30, 2016 at 16:25 UTC

• Attachment 0002-Removing-PROG-files-and-improving-code-structure.patch (raw) added

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

Changed by mooffie (@mooffie) on Jan 1, 2017 at 15:01 UTC (comment 9)

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.

Changed by mooffie (@mooffie) on Jan 3, 2017 at 14:20 UTC (comment 10)

• Blocked by set to #3751

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

Changed by mooffie (@mooffie) on Jan 16, 2017 at 16:00 UTC (comment 11)

Ok, we're ready to roll.

Changed by mooffie (@mooffie) on Jan 16, 2017 at 16:05 UTC (comment 12)

• Owner set to mooffie
• Milestone changed from Future Releases to 4.8.19
• Branch state changed from no branch to on review
• Status changed from new to accepted

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

branch: 3750_extfs_rpm_trans_scripts
Initial [4310b06]

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?)

Changed by mooffie (@mooffie) on Jan 16, 2017 at 16:08 UTC (comment 13)

• Votes set to 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.

Changed by mooffie (@mooffie) on Jan 16, 2017 at 16:11 UTC (comment 14)

@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"

Changed by jtyr (jiri.tyr@….com) on Jan 16, 2017 at 16:33 UTC (comment 15)

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.

Changed by zaytsev (@zyv) on Jan 16, 2017 at 18:39 UTC (comment 16)

(@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...

Changed by zaytsev (@zyv) on Jan 16, 2017 at 18:46 UTC (comment 17)

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.

Changed by zaytsev (@zyv) on Jan 16, 2017 at 19:01 UTC (comment 18)

• Votes changed from mooffie to mooffie zaytsev
• Branch state changed from on review to approved

Changed by mooffie (@mooffie) on Jan 17, 2017 at 10:24 UTC (comment 15.19)

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.

Changed by mooffie (@mooffie) on Jan 17, 2017 at 10:41 UTC (comment 20)

• Branch state changed from approved to merged
• Status changed from accepted to testing
• Resolution set to fixed
• Votes changed from mooffie zaytsev to committed-master

Merged to master: [43d7fce]

Changed by mooffie (@mooffie) on Jan 17, 2017 at 10:42 UTC (comment 21)

• Status changed from testing to closed

Changed by ossi (@ossilator) on Jan 22, 2017 at 9:13 UTC (comment 16.22)

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).

Changed by zaytsev (@zyv) on Jan 22, 2017 at 11:50 UTC (comment 23)

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

Changed by ossi (@ossilator) on Jan 23, 2017 at 8:52 UTC (comment 24)

i suspected that this might be the case. it would still be 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.

Changed by mooffie (@mooffie) on Jan 23, 2017 at 10:54 UTC (comment 23.25)

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.)

Changed by zaytsev (@zyv) on Feb 22, 2025 at 11:52 UTC

• Blocked by #3751 deleted

Changed by zaytsev (@zyv) on Feb 22, 2025 at 11:55 UTC (comment 27)

• Blocked by set to #3730