trunk icon indicating copy to clipboard operation
trunk copied to clipboard

Re-thinking configuration

Open ctron opened this issue 1 year ago • 18 comments

Right now there's a layer of configurations (order of priority, former will override latter):

  • CLI (clap)
  • Environment variables (mapped to serde with envy)
  • Trunk.toml config (serde)

The result is then passed on internally to structs for processing.

There are some areas of improvement:

  1. It doesn't always make sense to have the same behavior between CLI and config.
  2. CLI options sometimes influence how the configuration will be read
  3. environment variables get pushed into serde model, but actually are closer to the CLI model
  4. there are some inconsistencies (proxies, boolean flags not overriding false)

There are also some pending feature requests which seem to have an impact on the configuration:

  1. Profiles: It would be great if trunk would support build profiles. However, I would not really make sense to map those profiles to the CLI/env-vars. Only the selection should go there. However the profile selection should not be part of the serde configuration (also see: https://github.com/trunk-rs/trunk/issues/605).
  2. There was a discussion about adding the configuration to the Cargo.toml (personally I am not a fan of this, bet maybe we can have it both ways)
  3. Some parts of the build might want to have additional configuration (like minification level for CSS)
  4. There's the idea of plugins (has not manifested yet) But how could a plugin have its own configuration?

Bonus points:

  1. Allow other formats, beside TOML (I am looking at YAML). I would not force YAML on anyone, but with YAML it would be possible to leverage JSON schemas to enable IDE editor support.

ctron avatar Apr 19 '24 14:04 ctron

Why not eliminate the Trunk.toml entirely for Cargo.toml environment configuration? It serves virtually the same purpose, but the Trunk.toml adds noise to the root.

Other, mature bundlers, such as cargo-lambda for AWS, use this approach.

See https://www.cargo-lambda.info/commands/watch.html#environment-variables

For example, the Cargo.toml would look like this:

[package]
name = "basic-app"

[package.metadata.trunk.env]
RUST_LOG = "debug"
CARGO_PROFILE = "release"

[package.metadata.trunk.serve]
TRUNK_SERVE_ADDRESS = "127.0.0.1"
TRUNK_SERVE_PORT = 8080

I also think this fits better into the bevy ecosystem. In addition, environment variables are notoriously hard to work with in WASM, since they don't exist, but are still needed typically for things like deployments/environments, e.g. dev/stg/prod.

Today, if you want to have real deployment config for WASM apps, you have essentially three options:

Static:

pub const ENVIRONMENT = "dev";

Semi-static:

  • The problem with this approach is that, changing your environment variable requires a cargo clean and rebuild, since the environment variable is cached due to incremental builds.
pub const ENVIRONMENT = env!("ENV");

Fully dynamic, but requires features and special handling:

#[cfg(feature = "prod")]
pub const ENVIRONMENT = "prod";
#[cfg(feature = "stage")]
pub const ENVIRONMENT = "stg";
#[cfg(feature = "dev")]
pub const ENVIRONMENT = "dev";

simbleau avatar Apr 19 '24 16:04 simbleau

Why not eliminate the Trunk.toml entirely for Cargo.toml environment configuration? It serves virtually the same purpose, but the Trunk.toml adds noise to the root.

I personally see I right the opposite way. I think having "trunk" config in a "trunk config file" makes more sense, rather than adding noise to the "cargo config". And there are other examples that go exactly that way (cargo-embed, https://probe.rs/docs/tools/cargo-embed/#configuration).

So I definitely would not want to force people into either direction. If possible, enable the use of Cargo.toml, but not force it.

When talking about env-vars, that was more about the configuration, less about the actual code. Right now, instead of using CLI arguments, one can use env-vars to provide that configuration to trunk. I think that comes in handy when talking about CI. But I also think it is not super valuable on all cases. Then again, I just might not know what people use. So a zero effort approach (like clap provides) makes sense to me. People can use it, if it makes sense to them.

For having those deployment style options, I think it makes sense to go into the direction of "build profiles". Something similar to cargo profiles:

[profile.release]
minify = true
features = ["foo"]
[profile.release-stage]
inherits = "release"
minify = true
features = ["foo", "stage"]

That could be aligned with cargo profiles too. So that enabling profile release-stage would activate cargo profile release-stage too, unless overridden.

ctron avatar Apr 21 '24 09:04 ctron

re: Cargo.toml config - Personally I see more people using the Cargo.toml than a bespoke file. I think it should've been the default from the beginning, personally.

Examples:

I think we're the odd man out, or at the least should support Cargo.toml configuration.

For having those deployment style options, I think it makes sense to go into the direction of "build profiles". Something similar to cargo profiles:

[profile.release]
minify = true
features = ["foo"]
[profile.release-stage]
inherits = "release"
minify = true
features = ["foo", "stage"]

That could be aligned with cargo profiles too. So that enabling profile release-stage would activate cargo profile release-stage too, unless overridden.

Are cargo profile features a thing? I'm not seeing any documentation for profile features. A stack overflow issue 3 years ago suggests this isn't allowed, either. However if it does exist, I would be happy with that! #605 would solve my problems with multi-stage environments.

simbleau avatar Apr 21 '24 14:04 simbleau

Are cargo profile features a thing?

Not from cargo, but that would be the part the trunk would add. Still, selecting a trunk profile might also trigger the selection of a cargo profile. Maybe as opt-in, to not require a cargo profile for using trunk profiles.

I think when it comes to Cargo.toml vs Trunk.toml, you can find may examples for each side. But let's take a look at cargo-cross for example: https://github.com/cross-rs/cross?tab=readme-ov-file#configuration … It just gives you a bunch of options, and let's the user decide. And I think this is best approach. What's the least appealing option to me would be to force people into rewriting their existing Trunk.toml. Having tutorials out there which mention Trunk.toml, that would no longer be supported.

ctron avatar Apr 24 '24 06:04 ctron

Let's plan to support both.

As far as the cargo features go, I want to avoid confusion. Anything that would exist in the Trunk.toml should preface with package.metadata.trunk,

// Cargo.toml
[profile.release]
minify = true

[package.metadata.trunk.profile.release]
features = ["prod"]

[profile.release-stage]
inherits = "release"
minify = true

[package.metadata.trunk.profile.release-stage]
features = ["stage"]

On the Trunk.toml, though, we don't have to do that:

// Cargo.toml
[profile.release]
minify = true

[profile.release-stage]
inherits = "release"
minify = true
// Trunk.toml
[profile.release]
features = ["prod"]

[profile.release-stage]
features = ["stage"]

simbleau avatar Apr 24 '24 13:04 simbleau

Yup, that sounds great. Personally I would also be keen on seeing:

# Trunk.yaml
$schema: "trunkrs.dev/config.schema.json"
profile:
  release:
     features: ["prod"]
  release-stage:
     features: ["prod"]

Assuming that's added by only a few lines of extra code. But the focus, for sure, should be on Cargo.toml and Trunk.toml.

ctron avatar Apr 29 '24 06:04 ctron

Thinking about "when", I would prefer to finish up 0.20.0 first, and then work on that for 0.21.x? Would that work for you?

ctron avatar Apr 29 '24 06:04 ctron

Yup, that sounds great. Personally I would also be keen on seeing:

# Trunk.yaml
$schema: "trunkrs.dev/config.schema.json"
profile:
  release:
     features: ["prod"]
  release-stage:
     features: ["prod"]

Assuming that's added by only a few lines of extra code. But the focus, for sure, should be on Cargo.toml and Trunk.toml.

I'd prefer a version: 1 or something in the YAML, since the URL contents can change.

simbleau avatar Apr 29 '24 22:04 simbleau

Thinking about "when", I would prefer to finish up 0.20.0 first, and then work on that for 0.21.x? Would that work for you?

Sure, this feels like a great change. Probably the highest on my priority list, among workspace support.

simbleau avatar Apr 29 '24 22:04 simbleau

Yup, that sounds great. Personally I would also be keen on seeing:

# Trunk.yaml
$schema: "trunkrs.dev/config.schema.json"
profile:
  release:
     features: ["prod"]
  release-stage:
     features: ["prod"]

Assuming that's added by only a few lines of extra code. But the focus, for sure, should be on Cargo.toml and Trunk.toml.

I'd prefer a version: 1 or something in the YAML, since the URL contents can change.

Well, that would not allow editor support. And that's the whole idea behind that schema. Being able to auto-complete configurations.

ctron avatar May 03 '24 05:05 ctron

Yup, that sounds great. Personally I would also be keen on seeing:

# Trunk.yaml
$schema: "trunkrs.dev/config.schema.json"
profile:
  release:
     features: ["prod"]
  release-stage:
     features: ["prod"]

Assuming that's added by only a few lines of extra code. But the focus, for sure, should be on Cargo.toml and Trunk.toml.

I'd prefer a version: 1 or something in the YAML, since the URL contents can change.

Well, that would not allow editor support. And that's the whole idea behind that schema. Being able to auto-complete configurations.

Sounds good- We could do trunkrs.dev/config.schema.v1.json then?

simbleau avatar May 03 '24 14:05 simbleau

Btw, I am slowly working towards this (as time permits). There's nothing to share yet, but I'll share a PR as soon as there is something that compiles.

ctron avatar May 08 '24 07:05 ctron

There's an initial draft PR. It's not finished and lacks Cargo.toml support. Although that's prepared. If you're curios, maybe take a look at https://github.com/trunk-rs/trunk/pull/795 … if want to try out something, maybe wait a bit longer ;)

ctron avatar May 22 '24 07:05 ctron

Loving the progress @ctron ! This has, by far, been the most requested thing I've wanted for years. :)

simbleau avatar May 22 '24 13:05 simbleau

Can this be closed since #795 was merged?

NiklasEi avatar Jul 09 '24 21:07 NiklasEi

I am not 100% sure where are there yet. So the answer is: maybe :) But let's keep it open until it's released and everyone (mostly everyone … some people) are happy.

ctron avatar Jul 10 '24 06:07 ctron

I think https://github.com/trunk-rs/trunk/issues/605 is not yet fixed by https://github.com/trunk-rs/trunk/pull/795?

benfrankel avatar Jul 17 '24 01:07 benfrankel

I think #605 is not yet fixed by #795?

That's right, the work @ctron did to re-work configuration was for configuration maintainability.

I believe #719 and #605 is still much needed quality of life, and planned, but I don't have bandwidth to support that development at the moment.

simbleau avatar Jul 18 '24 23:07 simbleau