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

track and clean output files for builds #1291

Merged
merged 1 commit into from
Jan 29, 2021

Conversation

bcressey
Copy link
Contributor

Issue number:
Fixes #1027

Description of changes:
Add a layer of indirection between the build artifacts and the final output directory, so we can keep track of what we've previously built for a given package or variant.

That allows us to remove the files before the next build, so they do not interact with other builds in unexpected ways.

Testing done:
Verified that the state directory is populated with marker files.

❯ find build/state/variants -type f
build/state/variants/aws-k8s-1.17/bottlerocket-aws-k8s-1.17-aarch64-1.0.5-e3234419-data.img.lz4.marker
build/state/variants/aws-k8s-1.17/bottlerocket-aws-k8s-1.17-aarch64-1.0.5-e3234419-root.ext4.lz4.marker
build/state/variants/aws-k8s-1.17/bottlerocket-aws-k8s-1.17-aarch64-1.0.5-e3234419-migrations.tar.marker
build/state/variants/aws-k8s-1.17/bottlerocket-aws-k8s-1.17-aarch64-1.0.5-e3234419-boot.ext4.lz4.marker
build/state/variants/aws-k8s-1.17/bottlerocket-aws-k8s-1.17-aarch64-1.0.5-e3234419.img.lz4.marker
build/state/variants/aws-k8s-1.17/bottlerocket-aws-k8s-1.17-aarch64-1.0.5-e3234419-root.verity.lz4.marker

Confirmed that the corresponding files in packages and images are removed before another build. For example, old variant images are cleaned up on each build, so we no longer accumulate artifacts named for older commits. I also bumped the release package to version 1000.0, built it, then reverted it to 0.0. The older package with the higher version number was cleaned up as expected.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

Copy link
Contributor

@webern webern left a comment

Choose a reason for hiding this comment

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

So, because we rely on Cargo to detect changes, buildsys knows that it must rebuild, but sometimes it doesn't because rpmbuild sees the output files? Therefore you we now always delete those output RPM files? Do I understand correctly? Thanks.

tools/buildsys/src/builder.rs Show resolved Hide resolved
BuildType::Variant => "variants",
};

let path = [&getenv("BUILDSYS_STATE_DIR")?, prefix, name]
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point it might be nice to move all of these variables to a StructOpt, which can get its values from environment variables. This would make it easier to understand all of the inputs to the program by centralizing them.

@bcressey
Copy link
Contributor Author

So, because we rely on Cargo to detect changes, buildsys knows that it must rebuild, but sometimes it doesn't because rpmbuild sees the output files? Therefore you we now always delete those output RPM files? Do I understand correctly?

buildsys would always rebuild, but the output files from previous builds were not cleaned up, and accumulated in the build/rpms directory, and continued to be added to a repository for dnf to consume.

In the related issue, we observed that if we updated glibc to a newer version, then reverted that change because of a problem we found during testing, the newer versioned RPMs would still be there, and dnf would continue to install them instead of the most recently built RPMs, since package managers care about versions rather than timestamps.

Deleting the RPMs avoids the mismatch between what we expect the build system to do - packages and images should always use the most recently built dependencies - and what it actually does.

Copy link
Contributor

@tjkirch tjkirch left a comment

Choose a reason for hiding this comment

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

I don't want to block, but I do think documentation is important here. The process makes some assumptions about the outputs of builds, and is something that could have subtle effects we're not anticipating.

Personally, I'm also worried about the removal of old artifacts, because of the impact to developer build time. I don't think working on different branches should mean abandoning prior artifacts if relevant sources on the given branch haven't changed. We should be able to figure out use of the latest artifacts, but I know it's a hard problem, and we're solving something else here.

tools/buildsys/src/builder/error.rs Outdated Show resolved Hide resolved
tools/buildsys/src/builder.rs Outdated Show resolved Hide resolved
tools/buildsys/src/builder.rs Outdated Show resolved Hide resolved
@bcressey
Copy link
Contributor Author

Personally, I'm also worried about the removal of old artifacts, because of the impact to developer build time. I don't think working on different branches should mean abandoning prior artifacts if relevant sources on the given branch haven't changed. We should be able to figure out use of the latest artifacts, but I know it's a hard problem, and we're solving something else here.

Essentially this code only runs when cargo has already decided that a package needs to be rebuilt. That should always happen today when switching between branches where the files for a package have changed. If they haven't changed, then cargo should not see any modifications and won't start a rebuild.

If you're seeing spurious package rebuilds, it's possible we have another issue like the one in #996 that needs to be root caused.

tools/buildsys/src/builder.rs Outdated Show resolved Hide resolved
This adds a layer of indirection between the build artifacts and the
final output directory, so we can keep track of what we've previously
built for a given package or variant.

That allows us to remove the files before the next build, so they do
not interact with other builds in unexpected ways.

Signed-off-by: Ben Cressey <[email protected]>
@bcressey bcressey merged commit 7a2f125 into bottlerocket-os:develop Jan 29, 2021
@bcressey bcressey deleted the build-cleanup branch January 29, 2021 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

newer versions of RPMs will be used instead of the most recent builds
3 participants