twoliter: twoliter build alpha
#108
Conversation
ecpullen
force-pushed
the
alpha-build
branch
3 times, most recently
from
1984406 to
f5ce77f
Compare
twoliter/src/cmd/build.rs
Outdated
| /// The go modules that should be build | ||
| #[clap(long = "go-modules", env = "GO_MODULES", default_value = "")] | ||
| go_modules: String, |
There was a problem hiding this comment.
I'd rather not expose this here. Potentially we want a contract like:
Twoliter will search the
sourcespath forCargo.lockandgo.modfiles and download the referenced Rust crates and Go modules prior to package builds.
And eventually support for .twoliterignore files to suppress this behavior by excluding certain paths.
There was a problem hiding this comment.
I'd rather not expose [Go Modules] here. Potentially we want a contract like:
This is tricky because it either has to be implemented now, or there has to be a way to provide the list of Go modules (because the build system currently operates using a list of Go modules, effectively hard-coded in the Makefile, but which can be overridden).
A possible middle ground would be to include it here with a high priority follow-up issue to remove it.
There was a problem hiding this comment.
Oh look, I have a very well-written (:sweat_smile: ) issue for this already. #88
ecpullen
force-pushed
the
alpha-build
branch
2 times, most recently
from
a71af5c to
a8939e4
Compare
There was a problem hiding this comment.
Looks good, mostly a thought here about not using bash in the Makefile for go module resolution. I also don't like the fact that we are forcing users to have a flat directory structure for their go modules, however that is currently a limitation of downstream logic in the Makefile.toml.
One thing we could do is make the GO_MODULES variable a list of directory paths and change the downstream Makefile.toml logic to be less strict about them being top level sub-directories of sources.
| @@ -97,7 +97,8 @@ BUILDSYS_JOBS = "8" | |||
| CARGO_HOME = "${BUILDSYS_ROOT_DIR}/.cargo" | |||
| # This needs to end with pkg/mod so that we can mount the parent of pkg/mod as GOPATH. | |||
| GO_MOD_CACHE = "${BUILDSYS_ROOT_DIR}/.gomodcache/pkg/mod" | |||
| GO_MODULES = "ecs-gpu-init host-ctr" | |||
| # Dynamically load a list of go modules from ${BUILDSYS_SOURCE_DIR} | |||
| GO_MODULES = { script = ['find ${BUILDSYS_SOURCES_DIR} -name go.mod -type f -printf "%h\n" | xargs -n1 basename'] } | |||
There was a problem hiding this comment.
Do we want to do this with Rust prior to the calling of cargo make and pass the found go modules with -e GO_MODULES? I'm thinking now is as good a time as any to start moving logic away from Makefile.toml (or at least not adding more logic to it).
| for file_name in created_files { | ||
| let added = Path::new(&file_name); | ||
| if added.is_file() { | ||
| remove_file(added).await?; | ||
| } else if added.is_dir() { | ||
| remove_dir_all(added).await?; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm a bit skeptical of this. The frequency with which it will never run (a previous error occurred, or the user cancelled the program), and that all it contains is the directory sources/models makes me think it's fine to just leave these cryptic directories in place for alpha.
There was a problem hiding this comment.
Does it hurt to try to clean it up?
There was a problem hiding this comment.
I guess not, other than delaying the surprise of finding them until an error or cancelled execution has occurred.
ecpullen
force-pushed
the
alpha-build
branch
from
cebee93 to
337b5a0
Compare
| image.to_string(), | ||
| ]; | ||
|
|
||
| exec(Command::new("docker").args(args), true).await?; |
There was a problem hiding this comment.
This gets repeated often enough that a docker_exec function that just took args might be useful.
| // TODO: Remove once models is no longer conditionally compiled. | ||
| // Create the models directory for the sdk to mount | ||
| let models_dir = project.project_dir().join("sources/models"); | ||
| if !models_dir.is_dir() { | ||
| debug!("models source dir not found. Creating a temporary directory"); | ||
| fs::create_dir_all(&models_dir.join("src/variant")) | ||
| .context("Unable to create models source directory")?; | ||
| created_files.push(models_dir) | ||
| } |
There was a problem hiding this comment.
Can we mkdir -p this inside the embedded Makefile.toml so that the directory only gets created in the build context, not out on the host? That would eliminate the need for the created files cleanup later.
ecpullen
force-pushed
the
alpha-build
branch
from
337b5a0 to
66ba51f
Compare
Issue number:
Closes #104
Closes #82
Closes #80
Closes #79
Description of changes:
Testing done:
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.