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

Re-thinking configuration #780

Open
ctron opened this issue Apr 19, 2024 · 14 comments
Open

Re-thinking configuration #780

ctron opened this issue Apr 19, 2024 · 14 comments
Labels
help wanted Extra attention is needed
Milestone

Comments

@ctron
Copy link
Collaborator

ctron commented Apr 19, 2024

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: Configuration to compile Rust with any profile #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 ctron added help wanted Extra attention is needed needs design This item needs design work labels Apr 19, 2024
@simbleau
Copy link
Member

simbleau commented Apr 19, 2024

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";

@ctron
Copy link
Collaborator Author

ctron commented Apr 21, 2024

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.

@simbleau
Copy link
Member

simbleau commented Apr 21, 2024

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.

@ctron
Copy link
Collaborator Author

ctron commented Apr 24, 2024

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.

@simbleau
Copy link
Member

simbleau commented Apr 24, 2024

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"]

@ctron
Copy link
Collaborator Author

ctron commented Apr 29, 2024

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
Copy link
Collaborator Author

ctron commented Apr 29, 2024

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?

@simbleau
Copy link
Member

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
Copy link
Member

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.

@ctron
Copy link
Collaborator Author

ctron commented May 3, 2024

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 ctron added this to the 0.21.0 milestone May 3, 2024
@simbleau
Copy link
Member

simbleau commented May 3, 2024

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?

@ctron
Copy link
Collaborator Author

ctron commented May 8, 2024

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 ctron removed the needs design This item needs design work label May 22, 2024
@ctron
Copy link
Collaborator Author

ctron commented May 22, 2024

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 #795 … if want to try out something, maybe wait a bit longer ;)

@simbleau
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants