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

feat: configure-time-toggles (cont) #10811

Merged

Conversation

Leonidas-from-XIV
Copy link
Collaborator

This is a continuation of @emillon's work in #10724; I've rebased that PR and removed the witnesses by adding the toggles to a second-non heterogenous list of config options that only contains the toggles.

I am not sure this was the direction Etienne wanted to take it, so I am creating a new PR for it.

@rgrinberg
Copy link
Member

Are you planning on modifying the defaults for any existing configuration values? If not, I don't see why any modifications to dune_config are necessary.

@gridbugs
Copy link
Collaborator

Are you planning on modifying the defaults for any existing configuration values? If not, I don't see why any modifications to dune_config are necessary.

For the developer preview we'll be enabling toolchains by default. There's are a few more features currently in progress that will also be disabled by default on the main branch and opam releases of dune but enabled by default in the developer preview.

@Leonidas-from-XIV Leonidas-from-XIV force-pushed the compile-time-toggles branch 2 times, most recently from 7cd0ed6 to 989e4ec Compare August 13, 2024 09:07
Signed-off-by: Marek Kubica <marek@tarides.com>
@Leonidas-from-XIV Leonidas-from-XIV merged commit 7ef8c00 into ocaml:main Aug 13, 2024
27 of 28 checks passed
@Leonidas-from-XIV Leonidas-from-XIV deleted the compile-time-toggles branch August 13, 2024 09:57
@rgrinberg
Copy link
Member

rgrinberg commented Aug 13, 2024

There's couple of things here that should have never been merged:

  1. the changes to dune_config
  2. the changes to dune_engine

rgrinberg added a commit that referenced this pull request Aug 13, 2024
rgrinberg added a commit that referenced this pull request Aug 13, 2024
@@ -67,3 +69,5 @@ val threaded_console_frames_per_second : [ `Default | `Custom of int ] t
Note that environment variables take precedence over the values passed here
for easy overriding. *)
val init : (Loc.t * string) String.Map.t -> unit

val party_mode : Toggle.t t
Copy link
Member

Choose a reason for hiding this comment

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

This is just a dummy config, do you still need it after testing?

Copy link
Collaborator Author

@Leonidas-from-XIV Leonidas-from-XIV Aug 13, 2024

Choose a reason for hiding this comment

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

No, this is just for testing, it can safely be removed. I haven't considered it to be of particular importance but it can be removed without any problem.

@rgrinberg
Copy link
Member

In particular, here's a way to accomplish what you need without touching dune_config (which we are using internally):

  1. Introduce individual flags for all the options you would like to set the default for at configure time
  2. Use the options defined in 1. as the defaults when defining your config values

Everything else seems unnecessary

anmonteiro pushed a commit to anmonteiro/dune that referenced this pull request Nov 17, 2024
Signed-off-by: Marek Kubica <marek@tarides.com>
Co-authored-by: Etienne Millon <me@emillon.org>
anmonteiro pushed a commit to anmonteiro/dune that referenced this pull request Nov 17, 2024
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.

4 participants