Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prepare dist-git sources using rpmbuild -bp #2641

Merged
merged 1 commit into from
Mar 25, 2024
Merged

Conversation

lukaszachy
Copy link
Collaborator

@lukaszachy lukaszachy commented Jan 24, 2024

I had to split DiscoverFMF.go() to have something to call from prepare step, could be a conflict with #2448.

Also I've duplicated GuestPackageManager into PrepareDistGit so this shouldn't be blocked by #2557 - Once that PR is merged I will change the code to be nicer but lets get this working ...

Feedback wanted mostly for:

  • Added discover.post_dist_git method as way to run test discovery from prepare
  • What can / should be done about printing accurate numbers of discovered tests as we know the number only in prepare step (no way to update previous step)

Pull Request Checklist

  • implement the feature
  • write the documentation
  • extend the test coverage
  • update the specification
  • adjust plugin docstring
  • modify the json schema
  • mention the version
  • include a release note

@lukaszachy
Copy link
Collaborator Author

Note: when squashing - reference #1482

tmt/steps/discover/fmf.py Outdated Show resolved Hide resolved
@lukaszachy lukaszachy added this to the 1.31 milestone Jan 30, 2024
@lukaszachy
Copy link
Collaborator Author

@happz finally I'm down to single mypy complain. But I can't fix it, any help greatly appreciated

tmt/steps/prepare/distgit.py Outdated Show resolved Hide resolved
@lukaszachy
Copy link
Collaborator Author

Rebased to the current main (mainly to have PROVISION_HOW available)

@psss
Copy link
Collaborator

psss commented Feb 27, 2024

Seems that el9 packages failed to be built and core test plan failed.

@lukaszachy
Copy link
Collaborator Author

issues should be fixed now

@lukaszachy
Copy link
Collaborator Author

lukaszachy commented Feb 28, 2024

I've omitted @tmt.steps.provides_method and now PrepareDistGit is not available in --help menu.
I believe this isn't step one should run manually.

@lukaszachy lukaszachy marked this pull request as ready for review February 29, 2024 14:45
@lukaszachy
Copy link
Collaborator Author

I believe it is a good to go now (feature PoV). I'll start updating docs...

@lukaszachy lukaszachy added the ci | full test Pull request is ready for the full test execution label Feb 29, 2024
@lukaszachy
Copy link
Collaborator Author

Should dist-git-install-builddeps be true by default?

@lukaszachy
Copy link
Collaborator Author

Investigatig /tests/execute/upgrade failure... Error message is misleading - the missing path is Path('/var/tmp/tmt/run-333/plan/execute/upgrade-discover-remote/tests/var/tmp/tmt/run-333/plan/execute/upgrade-discover/tests') -- I need to find out why it is duplicated after dist-git changes in fmf.go()

Copy link
Collaborator

@psss psss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for implementing this! Looks good. Added some comments and suggestions.

tmt/steps/discover/__init__.py Outdated Show resolved Hide resolved
tmt/steps/discover/__init__.py Outdated Show resolved Hide resolved
tmt/steps/discover/__init__.py Outdated Show resolved Hide resolved
tmt/steps/discover/__init__.py Outdated Show resolved Hide resolved
tmt/steps/discover/shell.py Outdated Show resolved Hide resolved
tmt/steps/prepare/distgit.py Outdated Show resolved Hide resolved
tmt/steps/prepare/distgit.py Outdated Show resolved Hide resolved
tmt/steps/prepare/distgit.py Outdated Show resolved Hide resolved
tmt/steps/discover/fmf.py Outdated Show resolved Hide resolved
tmt/steps/discover/fmf.py Outdated Show resolved Hide resolved
@lukaszachy lukaszachy requested a review from psss March 19, 2024 13:23
@lukaszachy
Copy link
Collaborator Author

package-manager PR was merged so I'm going to incorporate it here.

Copy link
Collaborator

@psss psss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One typo.

tmt/steps/prepare/distgit.py Show resolved Hide resolved
tmt/steps/prepare/distgit.py Outdated Show resolved Hide resolved
@lukaszachy
Copy link
Collaborator Author

Using package_manager now and added coverage for 'install-builddeps' showed its implementation was buggy.

@lukaszachy
Copy link
Collaborator Author

Hit #2744 again.

@lukaszachy
Copy link
Collaborator Author

@psss Added test discovery: 2 tests selected it appears in prepare, example tmt run (no verbose):

/plan/fmf
    discover
        how: fmf
        directory: /home/lzachar/Doing/tmt-srpm-discovery
        Tests will be discovered after dist-git patching in prepare.
        summary: 0 tests selected
    provision
        queued provision.provision task #1: default-0
        
        provision.provision task #1: default-0
        how: container
        image: builder-fedora
        multihost name: default-0
        arch: x86_64
        distro: Fedora Linux 39 (Container Image)
    
        summary: 1 guest provisioned
    prepare
        queued push task #1: push to default-0
        
        push task #1: push to default-0
    
        queued prepare task #1: Prepare dist-git sources (buildrequires, patches, discovery...) on default-0
        queued prepare task #2: requires on default-0
        
        prepare task #1: Prepare dist-git sources (buildrequires, patches, discovery...) on default-0
            how: distgit
            name: Prepare dist-git sources (buildrequires, patches, discovery...)
            test discovery: 2 tests selected
        
        prepare task #2: requires on default-0
        how: install
        summary: Install required packages

@lukaszachy
Copy link
Collaborator Author

pre-commit find issue which will happen after the rebase. fixing.

@lukaszachy
Copy link
Collaborator Author

That package_manager fact is really flaky. dist-git patch requires 'dnf' (or 'dnf5') so the test failed because

        distro: Fedora Linux 39 (Cloud Edition)
        kernel: 6.5.6-300.fc39.x86_64
        package manager: yum

And later test failed on package_manager None which is the same issue - fact gathering failed (the order is dnf->yum) just for dist-git case the 'yum' probe was successful but in /prepare/ansible even this probe failed...

@happz
Copy link
Collaborator

happz commented Mar 21, 2024

That package_manager fact is really flaky. dist-git patch requires 'dnf' (or 'dnf5') so the test failed because

        distro: Fedora Linux 39 (Cloud Edition)
        kernel: 6.5.6-300.fc39.x86_64
        package manager: yum

And later test failed on package_manager None which is the same issue - fact gathering failed (the order is dnf->yum) just for dist-git case the 'yum' probe was successful but in /prepare/ansible even this probe failed...

Something fishy in the world of virtual... #2744 (comment)

Copy link
Collaborator

@psss psss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the adjustments! Looks good. Added some last fine tuning changes we discussed on the chat in 8047fe6.

@psss psss changed the title Prepare dist-git sources (rpmbuild -bp) Prepare dist-git sources using rpmbuild -bp Mar 21, 2024
@psss
Copy link
Collaborator

psss commented Mar 21, 2024

Hm, just tried a full test using the following plan in the tmt fedora distgit:

discover:
    how: fmf
    dist-git-source: true
    test: /tests/core/ls
execute:
    how: tmt

And it seems that the require is not properly handled:

prepare task #2: requires on default-0
how: install
summary: Install required packages
name: requires
order: 70
where: default-0
package: 1 package requested
    /usr/bin/flock

The test requires tmt package, but it is not included :-(

@lukaszachy
Copy link
Collaborator Author

lukaszachy commented Mar 22, 2024

Good catch, working on it:

  • Fix issue
  • Add require so it is tested in distgit.sh as well

@lukaszachy lukaszachy requested a review from psss March 22, 2024 15:35
Copy link
Collaborator

@psss psss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, now works fine, just one suggestion/question.

tmt/steps/prepare/distgit.py Outdated Show resolved Hide resolved
@lukaszachy
Copy link
Collaborator Author

Thanks for the call, as mentioned I'll fix the 'list(set)' to make dependencies unique.
In the next release the refactor as #2784 will follow.

@psss psss self-assigned this Mar 25, 2024
@psss psss merged commit 1292ee0 into main Mar 25, 2024
19 of 20 checks passed
@psss psss deleted the prep-distgit-sources branch March 25, 2024 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci | full test Pull request is ready for the full test execution step | discover Stuff related to the discover step step | prepare Stuff related to the prepare step
Projects
None yet
Development

Successfully merging this pull request may close these issues.

discover using dist-git source should apply downstream patches before running tests
3 participants