buildsys: Add variant-paths command #107
Conversation
| @@ -98,7 +102,8 @@ USAGE: | |||
|
|
|||
| SUBCOMMANDS: | |||
| build-package Build RPMs from a spec file and sources. | |||
| build-variant Build filesystem and disk images from RPMs." | |||
| build-variant Build filesystem and disk images from RPMs. | |||
| variant-paths Get a list of all source paths that contribute to a variant." | |||
There was a problem hiding this comment.
What is interesting about this command is that it does not need to be invoked from a build.rs file. In my mind the purpose of buildsys is to be invoked by cargo from build.rs process. This command could be twoliter info variant-paths or something like that instead of being in buildsys. It would require moving common code somewhere or making a buildsys library.
There was a problem hiding this comment.
Oh wait, maybe it does. I need to look more closely.
There was a problem hiding this comment.
The idea was this could be invoked directly, but perhaps a twoliter info subcommand could be a better fit.
The motivation for buildsys was the existing logic in there for understanding Cargo.toml files and some of their relationships. The path identification is related but somewhat different than the existing code, so I don't think we ended up with as much overlap and mutually beneficial gains from being in buildsys though. It's still logically similar and easier to group that way, but I don't think it necessarily needs to be if there is a better place for this logic.
There was a problem hiding this comment.
The manifest parser is already a crate, and the spec parser and project walker could be crates too, if we want to shift this into twoliter or some other tool.
I would start by making it a standalone tool though, or keeping it in buildsys, and invoking via Makefile.toml. I want to make sure we have a consistent vision and agreement on what subcommands twoliter grows, since that CLI is our future API.
| // manifest.included_packages gives the RPM package names, which doesn't always match with the local | ||
| // package directory name. We need to use the [*dependencies] sections to find the real packages. | ||
| results.insert(manifest_dir.clone()); |
There was a problem hiding this comment.
Might be better to add a ManifestInfo::preferred_name() function that either returns package.metadata.build-package.package-name (if defined) or else package.name. And then update build_package to use that also.
tools/buildsys/src/main.rs
Outdated
| // Add our package dependencies | ||
| let dependencies = manifest.local_dependency_paths(&package_dir); | ||
| for dependency in dependencies { | ||
| if add_package_paths(root_dir, &dependency, paths).is_err() { | ||
| eprintln!("Error reading package information from '{}'", &dependency); | ||
| } | ||
| } |
There was a problem hiding this comment.
This ends up visiting and parsing the same manifest multiple times for dependencies like glibc that appear more than once in the graph.
A more efficient algorithm might be to add all buildsys package manifests paths in one pass, then process them all individually.
This adds a `buildsys variant-paths` command that gets all repo source paths that make up a given variant. The location of the repo to check, and the variant name to parse, are provided via the BUILDSYS_ROOT_DIR and BUILDSYS_VARIANT environment variables. Signed-off-by: Sean McGinnis <stmcg@amazon.com>
stmcginnis
force-pushed
the
variant-paths
branch
from
6c7a663 to
a7ec345
Compare
|
Closing this as no longer needed. With the removal of conditional compilation and the core kit migration, there's much less variant-specific code, and it only needs to be built once. |
Issue number:
N/A
Description of changes:
This adds a
buildsys variant-pathscommand that gets all repo source paths that make up a given variant.The location of the repo to check, and the variant name to parse, are provided via the BUILDSYS_ROOT_DIR and BUILDSYS_VARIANT environment variables.
Testing done:
Ran various permutations of command for different variants:
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.