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

Get started with lal easier #52

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bencord0
Copy link
Member

Remove the configs/ and remove the need to provide
a defaults file on first startup.

From a fresh checkout, can

$ cargo build
$ cargo run -- configure
$ cargo run -- build

This is now because the manifest file can contain
the environment definitions, and lal will fallback
to the user-wide environment definitions only if
not found (to preserve behaviour).

@bencord0 bencord0 force-pushed the environments-in-components branch 2 times, most recently from 17c264d to aa7ae35 Compare June 12, 2021 12:00
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

comments and a question

src/core/config.rs Show resolved Hide resolved
src/core/config.rs Outdated Show resolved Hide resolved
src/main.rs Outdated
@@ -270,13 +268,13 @@ fn handle_docker_cmds(
let xs = if a.is_present("parameters") {
a.values_of("parameters").unwrap().collect::<Vec<_>>()
} else {
vec![]
Vec::new()
Copy link
Member

Choose a reason for hiding this comment

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

these are equivalent, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Vec::new can be given in places that expect a FnOnce() -> Vec, and is a little bit easier on the compiler as it doesn't need to evaluate the macro.

I think what I actually want to do is this:

let xs: Vec<&str> = a.values_of("parameters")
    .map_or_else(Vec::new, |params| params.collect());

tests/common/mod.rs Outdated Show resolved Hide resolved
src/core/manifest.rs Show resolved Hide resolved
@bencord0
Copy link
Member Author

bencord0 commented Jun 16, 2021

environments at the manifest level?

Yeah, I want to decouple the repository's definition of the environment from the user's definition of the environment.

My intention is to not require the build environment be configured locally at all, and can be upgraded with the repository. This also helps keep the build environment needed for the code in sync through the code itself.

That said, I think that having locally defined the version of the environments is still useful as an override (because users could always maintain a local patch to the manifest's environment); the question then becomes one about precedence - should ~/.lal/config:environments override .lal/manifest.json:environments, or should it be the other way around.


On having the manifests across multiple repositories;

I think I'm okay with this sprawl. There are tools like https://github.com/Clever/microplane which have solve this problem for me in recent years.

abi compatibility between targets that are built in the same env

This is a really compelling argument for having the local config override the repo manifest's definition of the environment. I'll swap them around so that
a) existing users who have their local environment definitions setup continue to build in a consistent ABI
b) new users who don't have their environment defaults can build repositories that do have their own definitions.

  • Swap precedence of environments key. local overrides repo.
  • Add a tests for repo manifests without environments, builds should still pass if the environment is defined in ~/.lal/config.

@bencord0 bencord0 force-pushed the environments-in-components branch 3 times, most recently from 55780c0 to 8efe6b2 Compare June 20, 2021 17:27
Remove the `configs/` and remove the need to provide a defaults file on first startup.

From a fresh checkout

	$ cargo build
	$ cargo run -- configure
	$ cargo run -- build

The README has been updated with more examples.

This is now because the manifest file can contain the environment
definitions, and lal will fallback to the user-wide environment
definitions only if not found (to preserve behaviour).

As a consequence, the environment to build code can now be configured
with the code in `.lal/manifest.json`.
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.

None yet

2 participants