Ticket #3751 (closed task: fixed)

Opened 8 months ago

Last modified 7 months ago

Add a test for extfs's rpm helper

Reported by: mooffie Owned by: mooffie
Priority: major Milestone: 4.8.19
Component: mc-vfs Version: master
Keywords: Cc: jiri.tyr@…
Blocked By: #3753 Blocking: #3750
Branch state: merged Votes for changeset: committed-master

Description

I'm attaching a proposed test for the rpm helper.

There are two components here:

  • An independent tool, rpm2tags, which converts an RPM package to a parsable text file (a serialized hashtable). This lets our tests run even where the 'rpm' program isn't installed (I assume this is an objective of ours; maybe I'm wrong).
  • A code snippet that gets injected into the helper and imitates the 'rpm' program.

There's documentation in the commit messages and in the code itself so I'll stop here.

Change History

Changed 8 months ago by mooffie

Changed 8 months ago by mooffie

Changed 8 months ago by mooffie

comment:1 Changed 8 months ago by mooffie

Now, an obvious question is: "is this the right approach?"

A very cursory inspection of the patches may give an impression there's relatively too much code for too little. But a more careful inspection shows that:

  • The heart of rpm2tags is just 5 lines of code.
  • The injected shell snippet: its code is straightforward and no evisceration of the main code was needed.

The assumption was that we want tests to run without the 'rpm' binary. Even if we removed this condition, we'd still need some code, and documentation, and I'm not too sure we'd end up needing substantially less than in the proposed test.

In short: I myself am in favor of the proposed test.

Last edited 8 months ago by mooffie (previous) (diff)

comment:2 Changed 8 months ago by mooffie

  • Blocking 3750 added

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

comment:3 follow-up: ↓ 5 Changed 8 months ago by jtyr

You can use this spec file to build an RPM package with all possible flags:

https://gist.github.com/jtyr/b9d6abd0f2ad2c17daa75b02c8278d13

comment:4 Changed 8 months ago by jtyr

  • Cc jiri.tyr@… added

comment:5 in reply to: ↑ 3 Changed 8 months ago by mooffie

Replying to jtyr:

You can use this spec file to build an RPM package with all possible flags:


Thanks! it works.

(I'd better wait for Yury / Andrew to opine on this whole thing before I work on it further.)

comment:6 Changed 8 months ago by zaytsev

I've had a look at the code, and to me it looks great! I've noticed a typo in mcrpmfs_getDesription, btw, but then I realized it's likewise mistyped in the helper :-) That feeling when I wanted to criticize the injected script, and then realized I got it wrong...

Branch: 3751_extfs_rpm_tests
Initial changeset: 417917a324b43bf9cdeb41f88198881c2215f015

comment:7 follow-up: ↓ 9 Changed 8 months ago by andrew_b

What reason to place rpm2tags into helpers-list/misc subdirectory instead of directly in helpers-list/?
rpm2tags should be rpm2tags.in and use @PERL@ in shebang instead of direct /usr/bin/perl.
Probably, rpm.rewrite.sh also should use @PERL@ instead of perl.
rpm.rewrite.sh should be executable.

comment:8 Changed 8 months ago by mooffie

@Andrew: I'll reply to your comment later today. I didn't know about it before I prepared the following patch.

I'm attaching a patch that adds a test for @jtyr's contributed test.spec package.

(The next patch I intended to publish was to move the two files from 'misc/' to 'misc/rpm/', for the sake of tidyness. BTW, it's my plan someday to place each helper's tests in a separate folder, and when this day comes we'll get rid of the 'misc/rpm' folder by moving it there. However, since @Andrew wants me to modify or rename 'misc/rpm2tags', I'll have to delay that patch as it touches this path.)

comment:9 in reply to: ↑ 7 Changed 8 months ago by mooffie

Replying to andrew_b:

What reason to place rpm2tags into helpers-list/misc subdirectory instead of directly in helpers-list/?


  • It doesn't belong among "normal" files: it's an auxiliary tool: it's not run by any of MC's build / test / maintenance procedures.
  • It's not part of the test harness. It's out-of-place in that directory in this regard too.
  • I could have considered putting in helpers-list/ auxiliary tools that are relevant for all the helpers, but this one is specific to the rpm one. Putting it there will make helpers-list/ a cluttered "catch all" folder.

Ditto for @jtyr's contributed file, 'test.spec'. In my recent patch I placed it in that same 'misc' folder (which I intend to rename to 'misc/rpm'). I'm sure you'll agree that putting it too in helpers-list/ is inelegant.

Maybe I should have named that directory 'maint' instead of 'misc'? Would that have made the purpose of the files within clearer?

rpm2tags should be rpm2tags.in and use @PERL@ in shebang instead of direct /usr/bin/perl.


I agree that hardcoding '/usr/bin/perl' is not ok.

If you want me to have 'rpm2tags.in', just say so and I'll do it.

But I'd like to suggest an alternative: to use "#/usr/bin/env perl" instead. rpm2tags' use and audience is very different than the helpers' use and audience: it's supposed to be run, by hand, only by people creating new RPM tests. Why would this crowd have perl somewhere else than in PATH?

Probably, rpm.rewrite.sh also should use @PERL@ instead of perl.


Ok, I'll have to think of ways to solve this.

Let me think aloud about having 'rpm.rewrite.sh.in':

Cons:

  1. It's a bother working with *.in files.
  2. I'll need to tell the tester where the build tree is, so that it will be able to export, say,$MC_TEST_DATA_BUILD_DIR. Not that it's an issue, but I like the tests' current blessed ignorance of this matter.

Pros:

  1. In the future we may need other *.in files. Maybe not, maybe yes. So let's do it sooner instead of later.

An alternative solution is to rename the helper from 'rpm' to 'rpm.in' and to put @PERL@ there, which 'rpm.rewrite.sh' will then see.

Any thoughts, folks?

rpm.rewrite.sh should be executable.


No, it gets "sourced" into the helper (do "help ." in bash). (The ".sh" extension is there only for the sake of documentation, and to help editors detect the syntax.)

comment:10 Changed 8 months ago by mooffie

BTW, could anybody explain the rationale behind @PERL@ to me?

(Once I understand the rationale I may have another question: "does this rationale apply also for 'make check'?")

comment:11 Changed 8 months ago by mooffie

Ok, here are two patches to handle the perl path issue (this first one is a "fixup").

comment:12 Changed 8 months ago by mooffie

Finally, a patch to move misc/* to misc/rpm/*.

Changed 8 months ago by mooffie

comment:13 Changed 8 months ago by zaytsev

BTW, could anybody explain the rationale behind @PERL@ to me?

The main reason for shipping scripts (Perl, Python, Ruby) etc. as *.in files that get their shebangs expanded with the interpreter path is that the Linux default of "/usr/bin/xxx" doesn't apply to a whole host of Unix systems, as in BSDs like FreeBSD, SysV descendants like Solaris, and so on. In those cases "/usr/local/bin/perl" is the least surprising of all you can get, but think of Solaris (if you get your Perl from OpenCSW, it's "/opt/csw/bin/perl", whereas SunFreeware Perl goes to "/usr/local/bin/perl", and the list goes on), AIX, HP-UX and such.

Now the question is what's the problem with using /usr/bin/env? The answer is more subtle, and the point here is that distro maintainers totally hate it for package manager installed software (i.e. on BSD the ports system, or OpenCSW on Solaris), because it makes the behavior of such software unpredictable. As a distro packager, the only way of staying halfway sane is to be sure that interpreters for scripts shipped with your packages are picked from your supported pool of interpreters you build the software against.

Why is this important? Because often packaged software depends on the interpreter internals, e.g. ABIs for modules, like Python extensions, or some such things that change from version to version and are then autodiscovered at build time. But even if this is NOT the case, consider program FOO written in Python 2, which has #!/usr/bin/env python as a shebang and it's installed system-wide from a package manager. User A is not creative and is just using it normally. User B installed a custom built Python 3 interpreter in his $HOME/bin and because he wants Python 3 by default and is exceedingly stupid, he symlinked python3 -> python. Of course, all system-wide software using /usr/bin/env python is broken now and litters his console with exceptions. He calls up his friendly admin team explaining how badly the systems are set up. Of course, he rejects the notion that he has no clue about Unix and could have possibly done something wrong. A heated exchange ensues to the detriment of collegial relationships.

The way to avoid such problems (much more common that you would think in various forms, including the scenario above) is to hardcode reference interpreter in all packaged software.

Now, I've explained all this to clarify the background why certain things are done in certain ways. I'm not actually sure that the second part about /usr/bin/env applies to our case. The rpm2tags script is a maintainer thing, and I think using it there is completely safe. The tester things is more of a borderline case, but I can see the argument why one could use /usr/bin/env there as well: one can claim that tests are run on a reference system anyways, and thus one shouldn't expect any surprises there, and when they are run by a user, because the software was built from source, @PERL@ would resolve to his personal interpreter anyways. The @PERL@ thing is important for things like extfs scripts, but here, maybe /usr/bin/env can be used instead to not to mess with the substitutions. I haven't made up my mind and I need to go. What do you guys think?

comment:14 Changed 8 months ago by andrew_b

I think we should use the @XXX@ pattern for each/any program found via AC_PATH_PROG.

Last edited 8 months ago by andrew_b (previous) (diff)

comment:15 Changed 8 months ago by mooffie

@Yury: thanks a bunch! that explanation was very helpful.

As for rpm2tags: I'll make it use @PERL@. I don't think there's sense in this, but I want this ticket to move forward.

As for 'rpm.rewrite.sh' or any other script in the 'data' folder: it's easy to implement a mechanism to give us access to @PERL@ et al even in non *.in files. I'll publish a patch (in a separate ticket) next week. So, while my 3751-0006 patch already solved this issue, I'm now retracting that patch in favor of the (yet-to-be-posted) new mechanism.

So, to sum it up:

My last three attachments are canceled. (Is there a way to draw a strikethrough line, or other marking, over attachments here? I don't want to delete these files because they are still informative.)

comment:16 Changed 8 months ago by mooffie

  • Blocked By 3753 added

comment:17 Changed 8 months ago by mooffie

I'll resume working on this as soon as #3753 gets in.

As for rpm2tags: I'll make it use @PERL@. I don't think there's sense in this, but I want this ticket to move forward.


No, I've changed my mind: I'll simply rename it to rpm2tags.pl. Then we won't need the shebang. Why? Because otherwise I'll need to use that same uncomfortable xxx.in scheme for any other maintenance scripts we may add.

comment:18 Changed 7 months ago by mooffie

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

Ok, I've pushed an updated branch. It addresses the perl issue.

Branch: 3751_extfs_rpm_tests
Initial changeset: 76e4bcdd7c910e8377bc4ea23a17bc67df9bd492

This branch also incorporates the two new patches:

  • It adds a test for @jtyr's custom test.rpm (in addition to glib.rpm).
  • It moves misc/* to misc/rpm/*.
Last edited 7 months ago by mooffie (previous) (diff)

comment:19 Changed 7 months ago by mooffie

I'm attaching a 'diff' showing how exactly the perl issue was solved. In short:

  • 'rpm.rewrite.sh' now uses $PERL (thanks to #3753).
  • rpm2tags was renamed to rpm2tags.pl (and shebang line removed).

Changed 7 months ago by mooffie

comment:20 Changed 7 months ago by zaytsev

  • Votes for changeset set to zaytsev

LGTM, Andrew?

comment:21 Changed 7 months ago by andrew_b

  • Votes for changeset changed from zaytsev to zaytsev andrew_b
  • Branch state changed from on review to approved

comment:22 Changed 7 months ago by mooffie

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

comment:23 Changed 7 months ago by mooffie

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.