Fix fmf_root when using dist-git

[LecrisUT]

I don't think this is a suitable "fix", but it highlights the logic issue

• path is only used when dist-git-extract is specified
• dist-git-extract must be a directory in the current implementation. It does not behave as documented, i.e. to specify the sources to copy over One would expect in this case the root to be the same as if passed dist-git-extract, but it is actually flattened out
• path should support glob pattern because the extracted source is generally package_name-0.1.2

TODO:

• [ ] Document the spec file change of dist-git-extract in help, stories and cli

[LecrisUT]

/packit build

[thrix]

@LecrisUT hi, any luck with this?

[LecrisUT]

Sorry I have been caught up a bit and didn't get around to work on this. I'll try to do it this weekend. The main todo for this:

• [x] Document the variables dist-git-merge, dist-git-init, dist-git-extract, dist-git-remove-fmf-root in stories
• [x] Document the difference in the logic if dist-git-extract and/or path is specified or not in stories E.g. without dist-git-extract and path, it will default to use the first fmf_root it discovers, and use that as the root path moving on
• [x] Change the dist-git-extract logic Currently when using a dist-git-extract it uses the extracted path as the new root, e.g. for calculating path. That is unintuitive and it complicates the patching. Instead, the whole paths defined in dist-git-extract should be copied as is specified, preserving the path structure as it would be inside the rpm spec file
• [x] When no dist-git-extract is specified (i.e. when using auto-detected fmf_root) completely ignore path variable
• [x] If dist-git-extract = /, copy all extracted data and allow the usage of path
• [x] Use pathlib.Path instead of os.path.join. I didn't get around to figuring out the context for that and where it needs to be changed.

If someone can help with the first 2, that would be great. I always have a hard time coming up with words. If you want to help with the other ones, let me know and go ahead and push to my branch (or if you don't have write privilege submit a patch file or make a PR on my fork). I can comment on later in the PR.

[LecrisUT]

Ok, I have completely reworked how dist-git-extract:

• dist-git-extract always copies all files specified in the glob pattern, preserving the folder structure.
• dist-git-remove-fmf-root is ambiguous where/how to remove the fmf_root. For now I have it removed
• dist-git-init now initializes at the root of the extracted path

This one now needs some reviewing that it is implemented correctly and there is no edge-case that should change the logic implemented. Also there needs more tests implemented.

[psss]

Is this one still relevant? @lukaszachy, there were some recent changes in the dist-git implementation...

[therazix]

@lukaszachy Is this still relevant?

[LecrisUT]

The issue, yes, but the approach, no. To refresh on the original issue, you cannot specify where the tmt root is relative to the dist-git, leading to the need of a workaround like ^1

discover:
  how: fmf
  dist-git-source: true
  dist-git-extract: /
  path: spglib-2.6.0/test

Instead I would like it to be

discover:
  how: fmf
  dist-git-source: true
  # OptionA
  path: test
  # OptionB
  path: spglib-*/test

The thing that gets in the way is if you don't specify dist-git-extract, it would try to go to the first fmf root it finds and copy the fmf tree. The path value at that point is meaningless, so path needs to be accounted beforehand https://github.com/teemtee/tmt/blob/f3a0e53c633e727ded45cc7fbfd60f63b79c9487/tmt/steps/discover/fmf.py#L830-L840

But there are some other inconsistencies in the post_dist_git that randomly interfere.

[lukaszachy]

Apologies for taking too much time and introducing the mess in the first place. At this time, I think the best would be to clarify expected behavior, as current implementation is buggy, as I tried too much to avoid yet another option and didn't realize there can be more TMT_SOURCE directories...

The main problem is the ambiguity in --path which can be used as workaround to try local changes as opposed to --url.

Thus if the plan is set like:

discover:
  how: fmf
  dist-git-source: true
  url: https://src.fedoraproject.org/rpms/fmf

one could just swap url: with path: /tmp/local/fmf and try changes in the spec file (adding patches, etc.) without the need to push them to the remote. This is currenly broken and tmt requires setting the dist-git-merge option but the intention was like this.

Which brings the messy --dist-git-extract which was supposed to be the 'path' inside the extracted rpm sources to copy in and use in the discovery. I'm not sure anymore why I thought flattenning was the good idea, to be really frank. Probably to skip copying makefile/binary sources.

The --dist-git-init is a workaround to allow discovery if fmf root isn't part of the source tarball. The --dist-git-remove-fmf-root makes it possible to combine fmf trees.

I'd like to keep way to try local changes, but it doesn't need to be via path option as help message is really about the root. A dedicated option (and PR) will be more appropriate.

With all this background said, IMO use cases we need to support in tmt:

UC1 - source tarball provides both tmt metadata and test sources

Ideally dist-git-source: true is all one needs. tmt should be able to detect where the fmf root is (usually in ./tests, right) but path can be used for explicit location (glob supported). Only content of path (fmf root) is copied to the tests directory (respecting prune and type: file requires).

UC1.1 - source tarball provides tmt metadata and test sources but is missing fmf root

dist-git-init to create fmf root in directory specified by path option, otherwise same as UC1

UC2 - source tarball provides only test sources, metadata provided by plan's repo

dist-git-source: true and dist-git-merge: true.
How to specify which files are to be copied to the tests directory? Everything unless dist-git-extract is set?

UC3 - source tarball provides both tmt metadata and test sources, user wants to use metadata from plan instead

Partially solved by dist-git-remove-fmf-root but if there are multiple roots a better way is needed (#3155). Problem with specifying what to copy (possible job for dist-git-extract)

[LecrisUT]

The main problem is the ambiguity in --path which can be used as workaround to try local changes as opposed to --url.

Oh, interesting point. Indeed that's an interesting workaround that would be useful for me also. Maybe we can minimize the impact here if we add only 1 variable dist-git-extracted and have that as the override for skipping all dist-git operations. Alternatively, wouldn't tmt run discover completely override the step so the special handling of --path is not really an issue?

The workflow I have in mind is that the user would do all the extraction, patching and everything they want in the %prep, including any tmt init and at that point, there is no need for the original discover.how=fmf step and it can be overwritten completely.

With all this background said, IMO use cases we need to support in tmt:

UC1 - source tarball provides both tmt metadata and test sources

Ideally dist-git-source: true is all one needs. tmt should be able to detect where the fmf root is (usually in ./tests, right) but path can be used for explicit location (glob supported). Only content of path (fmf root) is copied to the tests directory (respecting prune and type: file requires).

Makes sense to me. Do we need it to be copied though? We already have the dist-git source available to all plan steps and further via TMT_SOURCE_DIR (although where this points is still ambiguous). Wouldn't it just work as if it was pointing to a different tmt tree?

My proposal on this is that TMT_SOURCE_DIR will contain everything as if %prep command was done and it would contain all the files under BUILD/%{name}-%{version}-build (i.e. you would usually have another %{name}-%{version} folder right under there). Then the path will be calculated as relative to TMT_SOURCE_DIR instead of relative to the current tree (there are actually other ambiguities of how path is calculated in other contexts that we should address/clarify at some point).

path may be written as a glob pattern, but it must expand to a single value

UC1.1 - source tarball provides tmt metadata and test sources but is missing fmf root

dist-git-init to create fmf root in directory specified by path option, otherwise same as UC1

Could we also assume we do not have .fmf in dist-git in this situation? Just test sources.

UC2 - source tarball provides only test sources, metadata provided by plan's repo

dist-git-source: true and dist-git-merge: true. How to specify which files are to be copied to the tests directory? Everything unless dist-git-extract is set?

In this case, I would expect files to be copied to the current tree. Here I would remove the dist-git-extract and replace it with path (aliasing for compatibility). path would have again the same semantics that it is relative to TMT_SOURCE_DIR and it accepts glob but must expand to a single value. The destination would always be the root of the current tree.

If the user needs more files, specific destination, or more complicated steps, they should use prepare + TMT_SOURCE_DIR instead, editing the TMT_TREE.

UC3 - source tarball provides both tmt metadata and test sources, user wants to use metadata from plan instead

Partially solved by dist-git-remove-fmf-root but if there are multiple roots a better way is needed (#3155). Problem with specifying what to copy (possible job for dist-git-extract)

I think this should be covered by plan.import instead. It would be too confusing that a discover step influences a plan's metadata. My thought for this is that it would only be applied under the $TMT_SOURCE_DIR/$path directory and would just complement UC2, although it doesn't have much impact if you do not make the destination configurable. They could also achieve the same effect with manual prepare + TMT_SOURCE_DIR steps.

---

Plan of action

Open a new clean PR and resolve the issue by:

1. Making sure TMT_SOURCE_DIR is available for all plan steps -> test steps
2. Decouple the path and make it relative to TMT_SOURCE_DIR
3. When dist-git-merge is set, use path as the "what to extract" again relative to TMT_SOURCE_DIR
4. Separate dist-git-prepare steps from the "re-run discover" step so that users have a hook to add steps in between these 2
5. Alias dist-git-extract to path and deprecate it
6. Deprecate dist-git-remove-fmf-root in favor of manual prepare steps

How does it sound?

[happz]

@lukaszachy please, check the https://github.com/teemtee/tmt/pull/2098#issuecomment-2883055478 comment, and share your thoughts on the proposed "plan of action".

[lukaszachy]

One big canned worm to decide first. Assuming url can be used to point to remote dist-git plan: Can we have plan using multiple dist-git configs?

Use case in mind: Integration of several components, among lines of

discover:
 - name: foo dependency
   how: fmf
   dist-git-source: true
   url: repo/foo.git
 - name: bar dependency
   how: fmf
   dist-git-source: true
   url: repo/bar.git

[lukaszachy]

• Making sure TMT_SOURCE_DIR is available for all plan steps -> test steps

Ack, although it might be an array. Thankfully $TMT_SOURCE_DIR should keep working for the most common 'single item' use case.

• Decouple the path and make it relative to TMT_SOURCE_DIR

Ack

• When dist-git-merge is set, use path as the "what to extract" again relative to TMT_SOURCE_DIR

Ack

• Separate dist-git-prepare steps from the "re-run discover" step so that users have a hook to add steps in between How will this be indicated on plan/CLI level? Seems very useful, not limited to dist-git. But it might be clashing with 'slicing' features, which happen in discover (right, @happz ?)

• Alias dist-git-extract to path and deprecate it

Ack

• Deprecate dist-git-remove-fmf-root in favor of manual prepare steps

Ack, it should be rare enough to let user do what they need, but ideally it will be just adding prepare.shell config to remove right files between 'rpmbuild -bp' and 're-run discover'.

[LecrisUT]

One big canned worm to decide first. Assuming url can be used to point to remote dist-git plan: Can we have plan using multiple dist-git configs?

Sounds good, but out-of-scope rn. There needs to be more discussion, e.g. how do we deal with TMT_SOURCE_DIR in that case. For now we could enforce that there must be only one dist-git step until we gather some use-cases for this.

https://github.com/teemtee/tmt/issues/3802 is also relevant for how we would want to make the transition

[psss]

Summary from the hacking session discussion: Let's try to implement the new behaviour and allow users to enable it by opting in. The old implementation would stay with us for some time to allow fluent transition.