Upgrade tdnf to version 3.5.8 and Fix the ptests#10464
Conversation
| name=basic | ||
| baseurl=http://localhost:8080/photon-test-src | ||
| -gpgkey=file:///etc/pki/rpm-gpg/VMWARE-RPM-GPG-KEY | ||
| +gpgkey=file:///etc/pki/rpm-gpg/MICROSOFT-RPM-GPG-KEY |
There was a problem hiding this comment.
For the key paths, it might be easier to instead link/copy them in the %check section to match the expectations from the tests. Specs tend to be more friendly to view if we ever change the key name/path and my hope is it'll be harder to miss this workaround at that time.
There was a problem hiding this comment.
in this sense are you saying to copy the microsoft gpg key to the vmware gpg key path?
There was a problem hiding this comment.
Yup, or just add a symbolic link before we run the tests.
There was a problem hiding this comment.
Symbolic link added in during check section.
| # remove problematic test file when running tests from within the rpm build directory | ||
| rm pytests/tests/test_srpms.py |
There was a problem hiding this comment.
Two questions here:
- What is wrong with these tests? I'm reluctant to remove tests without a good reason. If it's something on our end, we should fix it. If it's a different case, please add details as to why it's OK to remove these tests.
- Is this really ran from the
%buildstage? It would be ideal, if we could keep all test steps inside the%checksection, although I'm aware that's not always possible.
There was a problem hiding this comment.
The tests count and check the srpms present within the build expected RPM build directory. We don't necessarily know what exactly will be in this directory at the time of build, so these tests did not make much sense when this is the case and when being executed within the build directory. Additionally, the test's expected cleanup is to delete the entire directory. By doing so, the entire source directory and actively running tests are removed. Thus I've opted to remove thei file as to not be included prior to being built/included by cmake within the build directory.
Alternative to this, a skip directive could be added to each test, and the cleanup function could be removed as to no blow away the source. Though removing this in the spec %build or %prep section seemed much cleaner. I opted to include it within %build as it seemed more relevant to building rather than prepping to build.
There was a problem hiding this comment.
Got it. In that case, could I ask you to add that explanation you just gave here about why we need to remove the tests? A shorted version is fine, though.:)
And can you check, if moving this to the %check section is still fine? That way it's clearer this is a test-only change.
There was a problem hiding this comment.
I will check if its possible to remove during the check section, though I suspect it may have to remain in build or prep.
There was a problem hiding this comment.
Explanation added, also moved removal to check section. Since cmake duplicates the file in the build directory, it also is removed there for safety.
3 participants
Merge Checklist
All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)
*-staticsubpackages, etc.) have had theirReleasetag incremented../cgmanifest.json,./toolkit/scripts/toolchain/cgmanifest.json,.github/workflows/cgmanifest.json)./LICENSES-AND-NOTICES/SPECS/data/licenses.json,./LICENSES-AND-NOTICES/SPECS/LICENSES-MAP.md,./LICENSES-AND-NOTICES/SPECS/LICENSE-EXCEPTIONS.PHOTON)*.signatures.jsonfilessudo make go-tidy-allandsudo make go-test-coveragepassSummary
What does the PR accomplish, why was it needed?
Currently the ptests for tdnf don't run, also we are taking a minor upgrade for tdnf to go alongside the snapshot work. PR stops skipping tests, additionally makes them compatible with the chroot environment.
Change Log
Does this affect the toolchain?
YES
Test Methodology