feat: use vendored sources #648

mmartinv · 2024-03-15T10:43:41Z

fix: use official aws-nitro-enclaves-cose crate
fix: do not generate vendor tarball in builds
fix: lock clap_builder to 4.4 version
feat: use patched vendored sources for builds.
fix: update packit configuration

github-advanced-security

devskim found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

packit-as-a-service · 2024-03-15T11:00:08Z

Failed to load packit config file:

Cannot parse package config. ValidationError({'jobs': {0: {'packages': defaultdict(<class 'dict'>, {'fido-device-onboard-fedora': {'value': {'actions': ['Field may not be null.']}}, 'fido-device-onboard-centos': {'value': {'actions': ['Field may not be null.']}}})}, 1: {'packages': defaultdict(<class 'dict'>, {'fido-device-onboard-fedora': {'value': {'actions': ['Field may not be null.']}}, 'fido-device-onboard-centos': {'value': {'actions': ['Field may not be null.']}}})}, 2: {'packages': defaultdict(<class 'dict'>, {'fido-device-onboard-fedora': {'value': {'actions': ['Field may not be null.']}}})}, 3: {'packages': defaultdict(<class 'dict'>, {'fido-device-onboard-centos': {'value': {'actions': ['Field may not be null.']}}})}}, 'packages': defaultdict(<class 'dict'>, {'fido-device-onboard-fedora': {'value': {'actions': ['Field may not be null.']}}, 'fido-device-onboard-centos': {'value': {'actions': ['Field may not be null.']}}})})

For more info, please check out the documentation or contact the Packit team. You can also use our CLI command validate-config or our pre-commit hooks for validation of the configuration.

packit-as-a-service · 2024-03-15T11:00:12Z

Failed to load packit config file:

Cannot parse package config. ValidationError({'jobs': {0: {'packages': defaultdict(<class 'dict'>, {'fido-device-onboard-fedora': {'value': {'actions': ['Field may not be null.']}}, 'fido-device-onboard-centos': {'value': {'actions': ['Field may not be null.']}}})}, 1: {'packages': defaultdict(<class 'dict'>, {'fido-device-onboard-fedora': {'value': {'actions': ['Field may not be null.']}}, 'fido-device-onboard-centos': {'value': {'actions': ['Field may not be null.']}}})}, 2: {'packages': defaultdict(<class 'dict'>, {'fido-device-onboard-fedora': {'value': {'actions': ['Field may not be null.']}}})}, 3: {'packages': defaultdict(<class 'dict'>, {'fido-device-onboard-centos': {'value': {'actions': ['Field may not be null.']}}})}}, 'packages': defaultdict(<class 'dict'>, {'fido-device-onboard-fedora': {'value': {'actions': ['Field may not be null.']}}, 'fido-device-onboard-centos': {'value': {'actions': ['Field may not be null.']}}})})

For more info, please check out the documentation or contact the Packit team. You can also use our CLI command validate-config or our pre-commit hooks for validation of the configuration.

packit-as-a-service · 2024-03-15T11:00:14Z

Failed to load packit config file:

Cannot parse package config. ValidationError({'jobs': {0: {'packages': defaultdict(<class 'dict'>, {'fido-device-onboard-fedora': {'value': {'actions': ['Field may not be null.']}}, 'fido-device-onboard-centos': {'value': {'actions': ['Field may not be null.']}}})}, 1: {'packages': defaultdict(<class 'dict'>, {'fido-device-onboard-fedora': {'value': {'actions': ['Field may not be null.']}}, 'fido-device-onboard-centos': {'value': {'actions': ['Field may not be null.']}}})}, 2: {'packages': defaultdict(<class 'dict'>, {'fido-device-onboard-fedora': {'value': {'actions': ['Field may not be null.']}}})}, 3: {'packages': defaultdict(<class 'dict'>, {'fido-device-onboard-centos': {'value': {'actions': ['Field may not be null.']}}})}}, 'packages': defaultdict(<class 'dict'>, {'fido-device-onboard-fedora': {'value': {'actions': ['Field may not be null.']}}, 'fido-device-onboard-centos': {'value': {'actions': ['Field may not be null.']}}})})

For more info, please check out the documentation or contact the Packit team. You can also use our CLI command validate-config or our pre-commit hooks for validation of the configuration.

nullr0ute · 2024-03-15T12:51:12Z

.packit.yaml

          - fedora-latest
          - fedora-latest-aarch64
-    - job: copr_build
+          - centos-stream-9-x86_64


I don't believe we can use pakit for el9/c9s builds, we need various commit formats and approved bugs in commits.

Looks like we might be able to do EPEL builds for c9s? re: https://github.com/containers/podman/blob/main/.packit.yaml

We could do scratch EPEL builds, but the EPEL rules are that packages can't duplicate what's in RHEL so we can't package it there because it'll conflict and customers end up in a mess!

The rule makes sense, though I wonder how podman is getting around it.

For our purposes, it seems like skipping c9s and EPEL would be wise.

I am not sure I am following... The centos-stream9 builds are just builds triggered in packit's copr not Koji or any other downstream copr/dist-git or whatever so I don't think we need any commit formats/approved bugs or anything else. The packit produced RPMs will be useful to run the tests on the specific platforms and that what we needed, right?.

On the other hand, I might be wrong but the EPEL builds @miabbott is referring to are just copr builds of CentOS Stream 9 with the EPEL-next repo activated, nothing more, the resulting RPMs are not going to be in EPEL-next, they just need EPEL-next to be built. As we don't need any EPEL repository to build the FDO packages I will leave centos-stream9 as it is.

TBH, I'm not terribly familiar with how Packit handles c9s, so I was just looking for examples of other projects using it.

To that point, osbuild is using it for all downstream builds - https://github.com/osbuild/osbuild-composer/blob/main/.packit.yaml

And looking at their dashboard from Packit, shows that the c9s builds are just going to a copr - https://dashboard.packit.dev/projects/github.com/osbuild/osbuild

So I'll walk back my statement about skipping c9s...seems like it is something we can try. (We'll still need to do manual work to get "official" c9s builds made...or invest in a bot to do it for us like the osbuild folks have)

The rule makes sense, though I wonder how podman is getting around it.

Are they getting around it, I don't see any pakit builds in c9s for podman? https://kojihub.stream.rdu2.redhat.com/koji/packageinfo?packageID=1720

I am not sure I am following... The centos-stream9 builds are just builds triggered in packit's copr not Koji or any

So by centos-stream9 builds you're actually referring to copr builds against the c9s? I thought you meant as in c9s proper, when read "centos-stream9 builds" I read it as official c9s builds not copr el9 builds. Anything in copr is fine.

.packit.yaml

nullr0ute · 2024-03-15T12:56:26Z

data-formats/Cargo.toml

@@ -17,7 +17,7 @@ serde_cbor = "0.11"
 serde_repr = "0.1.6"
 serde_tuple = "0.5"
 thiserror = "1"
-aws-nitro-enclaves-cose = { git = "https://github.com/nullr0ute/aws-nitro-enclaves-cose/", rev = "e3938e60d9051690569d1e4fcbe1c0c99d2fafa8" }
+aws-nitro-enclaves-cose = "0.4.0"


so at which point do we fix the crate?

when running update-vendor.sh script

fido-device-onboard.spec

nullr0ute · 2024-03-15T13:03:04Z

fido-device-onboard.spec

 %if 0%{?rhel} >= 10
 %cargo_prep -v vendor
 %else
-%cargo_prep -V 1
+%cargo_prep -V 0


Generates the vendored source at rpm build time? In the case of RHEL/c9s/eln/Fedora that is not going to work as they don't have out bound connectivity. For copr builds it depends on the repo config but it should be fine for CI builds in copr.

From my tests it looks like the -V 1 specifies the source tarball containing the vendor files. Now those files are contained in the Source0 tarball and actually works

I don't believe we want it all as single tarball.

Could you please elaborate further on why we wouldn't want a single tarball?

We don't build rhel/c9s using the Makefile, we need an srpm and the vendored tarfile with the dependencies, which is uploaded to the build system.

We still need the vendored tarball in fedora for ELN builds don't we? and ELN is built from Fedora dist-git right? So we need the vendored tarball in all the distributions we build for, don't we?. What's the difference on shipping it in a separate tarball than alongside the source code? Other projects already do that like all the osbuild projects for example.

What do the Fedora rust packaging guidelines say? https://docs.fedoraproject.org/en-US/packaging-guidelines/Rust/

I would personally like them to be separate.

It literally says:

In general, packages SHOULD NOT use bundled crate dependencies, whenever possible.

So we are OK in Fedora as we don't use vendoring at all.

So using a separate upstream and vendored sourced allows me to:

Verify the upstream source hasn't changed from the github releases download to verify checksum etc

Refresh the vendored source against an upstream release without having to change the upstream release source to deal with CVEs in the vendored source.

Having them separate is useful for a number of reasons. Please just update this to keep them separate.

So using a separate upstream and vendored sourced allows me to:

Verify the upstream source hasn't changed from the github releases download to verify checksum etc

Sorry but I don't get what's the difference. If you include the vendor directory within the release you still can verify the checksum exactly the same way.

Refresh the vendored source against an upstream release without having to change the upstream release source to deal with CVEs in the vendored source.

Applying a CVE downstream without having to modify the upstream repo would be as simple as:

git clone https://github.com/fdo-rs/fido-device-onboard-rs.git cd fido-device-onboard-rs git checkout v0.5.0 # do the required modifications to address the CVE by changing dependencies and/or adding patches to the `patches` directory ./update-vendor.sh git diff > cve-number.diff

And use that cve-number.diff as patch in the dowstream repo.

Having them separate is useful for a number of reasons. Please just update this to keep them separate.

Sorry but I cannot update the PR to separate them, that's the whole point of this PR: to minimize hacks and to bring the filtered and patched vendor directory into the repo's source code. The only reason I see for now to not merge it is the problem I found regarding cargo add that I mentioned below. However that would be a minor problem that can be solved by:

rm -f .cargo/config.toml cargo add ... ./update-vendor.sh

7flying · 2024-03-19T09:00:14Z

I think that we are trying to accomplish too many things with these PR when we don't really need all those things. Here are my thoughts and what I understand to be "true"

First of all, as I understand it, this project simply needs to check that things work in Fedora, thereby, as it was before, we only care about making copr builds in Fedora rawhide and the latest Fedora development, both for aarch64 and x86_64.

We don't need to test that packages are built in rhel/c9s because we vendor everything, and since we compile everything ourselves """It Just Works""". It would be nice to have a rhel/c9s CI but as I've understood the PR, it does not replicate the way that we need to build things for rhel/c9s, so if it does not replicate it I would rather save time in CI and I wouldn't run rhel/c9s packaging tests.

When things do not work in rhel/c9s we make specific patches for it in the specific c9s repo, but for "upstream-sake" we had the make-vendored-tarfile script over here too. Due to the way that packaging works in rhel/c9s we do not need a Makefile that spits out an RPM, we need a sprm and a separate tarball with the vendored dependencies.
The only thing that I need is a better make-vendored-tarfile or X that filters the windows stuff better.

7flying · 2024-03-19T09:03:52Z

.cargo/config.toml

+replace-with = "vendored-sources"
+
+[source.vendored-sources]
+directory = "vendor"


this is going to use the vendored sourcesfor everyt build and we don't want that

Well that's not actually true, the %cargo_prep macro overwrites it. Only when %cargo_prep -V 0 or %cargo_prep -v vendor the build will use vendored sources.

Doing some additional tests I hit rust-lang/cargo#10729 so if we use vendoring as it is in this PR we will not be able to use cargo add anymore (at least in Fedora). I will change the strategy and remove the .cargo/config.toml and use a pre-commit script to update the vendor files whenever the Cargo.lock file is modified.

mmartinv · 2024-03-19T15:19:10Z

I think that we are trying to accomplish too many things with these PR when we don't really need all those things. Here are my thoughts and what I understand to be "true"

First of all, as I understand it, this project simply needs to check that things work in Fedora, thereby, as it was before, we only care about making copr builds in Fedora rawhide and the latest Fedora development, both for aarch64 and x86_64.

As we discussed in the refinement session we also need to enable CentOS Stream 9 builds at least.

We don't need to test that packages are built in rhel/c9s because we vendor everything, and since we compile everything ourselves """It Just Works""". It would be nice to have a rhel/c9s CI but as I've understood the PR, it does not replicate the way that we need to build things for rhel/c9s, so if it does not replicate it I would rather save time in CI and I wouldn't run rhel/c9s packaging tests.

We need to test the packaging itself and that the resulting RPMs work as expected. The way the RPMs are built in copr is basically the same as in dist-git I believe. Indeed, packit can propose downstream dist-git updates when a new version is released upstream so I think we could even automate some downstream work.

When things do not work in rhel/c9s we make specific patches for it in the specific c9s repo, but for "upstream-sake" we had the make-vendored-tarfile script over here too. Due to the way that packaging works in rhel/c9s we do not need a Makefile that spits out an RPM, we need a sprm and a separate tarball with the vendored dependencies. The only thing that I need is a better make-vendored-tarfile or X that filters the windows stuff better.

What's the difference compared to provide everything, the source code alongside the filtered and patched vendor files, in a single SRPM? Is there any problem I am not aware of? AFAIK osbuild guys already do that with osbuild-composer for example.

Use official `aws-nitro-enclaves-cose` crate instead of a fixed fork. Signed-off-by: Miguel Martín <mmartinv@redhat.com>

Do not generate the vendor tarball in builds as we will use the patched and filtered `vendor` dir instead Signed-off-by: Miguel Martín <mmartinv@redhat.com>

Use patched and filtered vendored sources for builds. Signed-off-by: Miguel Martín <mmartinv@redhat.com>

Update packit configuration to generate fedora, fedora-eln, centos and rhel packages. Add configuration to be able to propose downstream PR for Fedora dist-git. Signed-off-by: Miguel Martín <mmartinv@redhat.com>

github-advanced-security bot found potential problems Mar 15, 2024

View reviewed changes

mmartinv force-pushed the use-vendored-sources branch 2 times, most recently from 6c206b3 to 598d329 Compare March 15, 2024 11:00

mmartinv force-pushed the use-vendored-sources branch 7 times, most recently from d33cb89 to b9621db Compare March 15, 2024 12:56

nullr0ute reviewed Mar 15, 2024

View reviewed changes

mmartinv force-pushed the use-vendored-sources branch from b9621db to e04c458 Compare March 15, 2024 15:25

mmartinv requested review from nullr0ute and miabbott March 15, 2024 15:25

mmartinv force-pushed the use-vendored-sources branch 6 times, most recently from c8b2137 to 4d20924 Compare March 18, 2024 12:13

7flying reviewed Mar 19, 2024

View reviewed changes

mmartinv requested a review from 7flying March 20, 2024 09:47

mmartinv force-pushed the use-vendored-sources branch 3 times, most recently from e568337 to 792a69c Compare March 20, 2024 14:27

mmartinv force-pushed the use-vendored-sources branch 3 times, most recently from 26b10e6 to cd84756 Compare March 20, 2024 18:23

mmartinv added 2 commits March 25, 2024 16:27

fix: use official aws-nitro-enclaves-cose crate

d375324

Use official `aws-nitro-enclaves-cose` crate instead of a fixed fork. Signed-off-by: Miguel Martín <mmartinv@redhat.com>

fix: do not generate vendor tarball in builds

dec7a7c

Do not generate the vendor tarball in builds as we will use the patched and filtered `vendor` dir instead Signed-off-by: Miguel Martín <mmartinv@redhat.com>

mmartinv force-pushed the use-vendored-sources branch from cd84756 to 539fb0b Compare March 25, 2024 15:31

mmartinv added 2 commits March 25, 2024 16:33

feat: use patched vendored sources for builds

69b6d9e

Use patched and filtered vendored sources for builds. Signed-off-by: Miguel Martín <mmartinv@redhat.com>

fix: update packit configuration

afb61d7

Update packit configuration to generate fedora, fedora-eln, centos and rhel packages. Add configuration to be able to propose downstream PR for Fedora dist-git. Signed-off-by: Miguel Martín <mmartinv@redhat.com>

mmartinv force-pushed the use-vendored-sources branch from 539fb0b to afb61d7 Compare March 25, 2024 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use vendored sources #648

feat: use vendored sources #648

mmartinv commented Mar 15, 2024 •

edited

Loading

github-advanced-security bot left a comment

packit-as-a-service bot commented Mar 15, 2024

packit-as-a-service bot commented Mar 15, 2024

packit-as-a-service bot commented Mar 15, 2024

nullr0ute Mar 15, 2024

miabbott Mar 15, 2024 •

edited

Loading

nullr0ute Mar 15, 2024

miabbott Mar 15, 2024

mmartinv Mar 15, 2024

miabbott Mar 15, 2024

nullr0ute Mar 15, 2024

nullr0ute Mar 15, 2024

nullr0ute Mar 15, 2024

mmartinv Mar 15, 2024

nullr0ute Mar 15, 2024

mmartinv Mar 15, 2024

nullr0ute Mar 15, 2024

mmartinv Mar 18, 2024

7flying Mar 19, 2024

mmartinv Mar 19, 2024

nullr0ute Mar 19, 2024

mmartinv Mar 20, 2024

nullr0ute Mar 22, 2024

mmartinv Mar 25, 2024

7flying commented Mar 19, 2024

7flying Mar 19, 2024

mmartinv Mar 19, 2024

mmartinv Mar 20, 2024

mmartinv commented Mar 19, 2024

feat: use vendored sources #648

Are you sure you want to change the base?

feat: use vendored sources #648

Conversation

mmartinv commented Mar 15, 2024 • edited Loading

github-advanced-security bot left a comment

Choose a reason for hiding this comment

packit-as-a-service bot commented Mar 15, 2024

packit-as-a-service bot commented Mar 15, 2024

packit-as-a-service bot commented Mar 15, 2024

Choose a reason for hiding this comment

miabbott Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

7flying commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmartinv commented Mar 19, 2024

mmartinv commented Mar 15, 2024 •

edited

Loading

miabbott Mar 15, 2024 •

edited

Loading