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

Support for crate configuration #673

Merged
merged 1 commit into from
Mar 3, 2021
Merged

Support for crate configuration #673

merged 1 commit into from
Mar 3, 2021

Conversation

Fabien-Chouteau
Copy link
Member

@Fabien-Chouteau Fabien-Chouteau commented Jan 28, 2021

This is the possibility for crates to define configuration variables
that will then generate Ada, C, or GPR files based on the values set by
depending crates.

For embedded projects where compile optimization and static memory usage
are important, it is possible to define sizes of cache buffers or
maximum amount of some resources.

It allows to enable or disable features at compile time, such as debug
logging.

Example:

[config_variables]
Buffer_Size= {type="Integer", first=0, last=256, default = 20}
Freq = {type="Real", first=0.0, last=440.0, default=220.124}
Device_ID = {type = "String", default = "my device"}
Debug_Level = {type = "Enum", values=["Warn", "Info", "Debug"], default = "Warn"}
Print_Trace = {type = "Boolean", default = true}

[config_settings]
test.Buffer_Size= 100
test.Device_ID = "test config value"

test_config.ads:

--  Configuration for test generated by Alire
package test_Config is

   Device_ID : constant String := "test config value";

   Freq_First : constant := 0.0;
   Freq_Last : constant := 440.0;
   Freq : constant := 220.124;

   Print_Trace : constant Boolean := True;

   Buffer_Size_First : constant := 0;
   Buffer_Size_Last : constant := 256;
   Buffer_Size : constant := 100;

   type Debug_Level_Kind is (Warn, Info, Debug);
   Debug_Level : constant Debug_Level_Kind := Warn;

end test_Config;

Open questions:

  • Should it be possible to use configuration values in case? e.g.:
    [config_variables]
    Enable_Logs = {type = "Boolean"}
    
    [depends-on.case(Enable_Logs).True]
    logging = "1.0.0"

@mosteo
Copy link
Member

mosteo commented Jan 29, 2021

Looks great!

A bit of feedback on your open questions:

  • When should we first generate the configuration output (.ads, .gpr, .h)?

    • Before post-fetch?

Yes, and also before pre-build, in case there's been some manual edition.

  • Should it be possible to use configuration values in case? e.g.:
    [config_variables]
    Enable_Logs = {type = "Boolean"}
    
    [depends-on.case(Enable_Logs).True]
    logging = "1.0.0"

That's an amazing idea. How would conditional dependencies work with GPR files? In tandem with conditional project-files expressions?

Could this serve in the future to retrieve variants?: alr get somecrate --config Ssh_Enabled:=True

Back to earth, I'm unsure about the implementation difficulties, but my gut feeling is that it is reasonably doable. I would delay it though after #323, which should greatly simplify maintenance of all that part. Also the rework could be done with this idea in mind.

  • Should variable names be case sensitive? (they are right now)

I'd go with Ada syntax here, given the users expectations, hence no.

  • Use a different format for type definition?
    [config_variables]
    var1 = {type = "Real", First = "0.0", Last = "100.0", default = "50.0"}
    var2 = {type = "Enum", Values = ["A", "B", "C"], default = "B"}

My first impulse is to go for verbatim Ada syntax that you can stitch when generating the files, and no hidden assumptions. For example (also putting in a suggestion for the key naming):

# optional, only for new types:
[config.types] # everything after "Type X is". Probably [config.subtypes] would be needed too.
Log_Levels = "(Debug, Warn, Error)"
Buffer_Sizes = "new Integer range 128 .. 1024"

[config.variables] # everything after the colon in a declaration, default included
Buffer_Last = "constant Buffer_Sizes := 0"
Min_Level = "Log_Levels := Debug" # Constant optional?
Foo = 'constant String := "Bar"'

[config.settings] # override values for something existing in [config.variables]
crate.Min_Level = "Warn"
crate.Buffer_Last = "1024"

would generate

type Log_Levels is (Debug, Warn, Error);
type Buffer_Sizes is new Integer range 128 .. 1024;
Buffer_Last : constant Buffer_Sizes := 0;
Min_Level : Log_Levels := Warn;
Foo : constant String := "Bar";

This way you can also have several vars of the same type, and would not arbitrarily limit the kind of declarations you can use. May be a case of too much liberty though (although we could impose restrictions I guess, e.g. only literals for defaults).

Also, IIUC, normally one crate would define the types/defaults, and a dependent would set/override them, right? This means we would need to travel dependencies from leaves to root to properly override all the way up? Or at least detect conflicting settings?

@Fabien-Chouteau
Copy link
Member Author

Looks great!

Thanks Alejandro

  • When should we first generate the configuration output (.ads, .gpr, .h)?
    • Before post-fetch?
      Yes, and also before pre-build, in case there's been some manual edition.

Indeed.

  • Should it be possible to use configuration values in case? e.g.:
    That's an amazing idea. How would conditional dependencies work with GPR files? In tandem with conditional project-files expressions?

That a good question. Since gpr file do not have conditional with this could be a can of worm...

Could this serve in the future to retrieve variants?: alr get somecrate --config Ssh_Enabled:=True

I never thought about a command line interface for this.
But now that you are mentioning it we could ask users to provide values for variables that don't have default.

My first impulse is to go for verbatim Ada syntax that you can stitch when generating the files, and no hidden assumptions. For example (also putting in a suggestion for the key naming):

I just pushed a version that relies more on the TOML syntax. My rationale is that it is better to rely on the TOML library rather than implementing a parser just for this. And I do like the result.

# optional, only for new types:
[config.types] # everything after "Type X is". Probably [config.subtypes] would be needed too.
Log_Levels = "(Debug, Warn, Error)"
Buffer_Sizes = "new Integer range 128 .. 1024"

[config.variables] # everything after the colon in a declaration, default included
Buffer_Last = "constant Buffer_Sizes := 0"
Min_Level = "Log_Levels := Debug" # Constant optional?
Foo = 'constant String := "Bar"'

[config.settings] # override values for something existing in [config.variables]
crate.Min_Level = "Warn"
crate.Buffer_Last = "1024"

In my opinion this is going too far in complexity.

Also, IIUC, normally one crate would define the types/defaults, and a dependent would set/override them, right? This means we would need to travel dependencies from leaves to root to properly override all the way up? Or at least detect conflicting settings?

There is already a detection of conflicting settings. But right now I do not check if a crate only sets variable of its own dependencies, i.e. you can set values for crate you don't depend on.

@mosteo
Copy link
Member

mosteo commented Jan 29, 2021

Could this serve in the future to retrieve variants?: alr get somecrate --config Ssh_Enabled:=True

I never thought about a command line interface for this.
But now that you are mentioning it we could ask users to provide values for variables that don't have default.

I'm not so much concerned with the particular interface, but interested in this being an opportunity to allow packaging different minor variants without resorting to different crates.

My first impulse is to go for verbatim Ada syntax that you can stitch when generating the files, and no hidden assumptions. For example (also putting in a suggestion for the key naming):

I just pushed a version that relies more on the TOML syntax. My rationale is that it is better to rely on the TOML library rather than implementing a parser just for this. And I do like the result.

OK. Here, I was thinking on how to support easily things like delta, digits, etc, that seem important (from my inexperienced POV) for embedded.

In my opinion this is going too far in complexity.

Different personal perceptions, I guess. To me, closer to Ada is always more attractive. I was thinking in the interim to fully go to something like

config.declare = '''
< full Ada declarations >
'''

and just parse the type/var names and defaults from there (!). But no matter, if you like what you have.

There is already a detection of conflicting settings.

You're several steps ahead :)

@Fabien-Chouteau
Copy link
Member Author

OK. Here, I was thinking on how to support easily things like delta, digits, etc, that seem important (from my inexperienced POV) for embedded.

The code only generates named numbers with the idea that users can define their own types from them. This has to be explained the documentation, but so far I have this comment in the code generation part:

   --  For Real and Int we do not declare a ranged type such as:
   --
   --  type Test_Type is range 0 .. 100;
   --  Test : constant Test_Type := 50;
   --
   --  because this would limit the possibilities of users for things
   --  such as alignment size, etc. Instead we define named numbers
   --  (constant) for the First, Last and actual value of the
   --  variable:

   Test_First : constant := 0;
   Test_Last : constant := 100
   Test : constant := 50;

    --  User can then define their own types:
   type Test_Type is range Config.Test_First .. Config.Test_Last
     with Size => 32;

With this you can also define delta, digits, etc.

In my opinion this is going too far in complexity.

Different personal perceptions, I guess. To me, closer to Ada is always more attractive. I was thinking in the interim to fully go to something like

config.declare = '''
< full Ada declarations >
'''

I don't see the benefits of this? What would you generate?

@mosteo
Copy link
Member

mosteo commented Feb 1, 2021

OK, now I understand. So it's merely definition of constants and the types have to be explicitly written by the user, relying on these constants. Then I would not bother with assuming these constants are going to be used for ranges, and make the user to explicitly define whatever is needed (that is, give one definition for each of Foo_Min, Foo_Max... Or whatever the library needs).

My comments were on the assumption that the types would also be generated by Alire. But your idea is certainly simpler and flexible enough.

@Fabien-Chouteau
Copy link
Member Author

OK, now I understand. So it's merely definition of constants and the types have to be explicitly written by the user, relying on these constants.

I should have started with this.

Then I would not bother with assuming these constants are going to be used for ranges, and make the user to explicitly define whatever is needed (that is, give one definition for each of Foo_Min, Foo_Max... Or whatever the library needs).

The range comes from my first experiments in particular with buffer where you don't want negative sizes. So having an upper and lower bounds makes sense, and let the user know as soon as possible that the configuration not valid. I also think that max (and min?) string length would be nice.

My comments were on the assumption that the types would also be generated by Alire. But your idea is certainly simpler and flexible enough.

@Fabien-Chouteau
Copy link
Member Author

  • Should it be possible to use configuration values in case? e.g.:
    That's an amazing idea. How would conditional dependencies work with GPR files? In tandem with conditional project-files expressions?

That a good question. Since gpr file do not have conditional with this could be a can of worm...

Thinking again about this, we could change auto-gpr-with feature to generate withs in the XXX_config.gpr instead of the main gpr file. So the main gpr only has to with config/XXX_config.gpr.

doc/catalog-format-spec.md Show resolved Hide resolved
doc/catalog-format-spec.md Show resolved Hide resolved
@Fabien-Chouteau
Copy link
Member Author

@mosteo I will need your help to know where to call Root.Generate_Configuration;.

It basically has to be called when any of the crate manifests is changed because that could change the variable definitions or settings.

@mosteo
Copy link
Member

mosteo commented Feb 2, 2021

@mosteo I will need your help to know where to call Root.Generate_Configuration;.

It basically has to be called when any of the crate manifests is changed because that could change the variable definitions or settings.

For the first generation after retrieval of a crate, around here would be the place:

-- Run actions on first retrieval

Problem is, this won't update the root crate after its manifest is edited, as this is a new necessity. (Until now, manifest edition could only affect dependencies). For regenerating the root crate config, the proper place is around here:

Finally, there's the problem of linked crates, which are likely targets for edition. These are currently skipped as they don't need deployment. (Side note: This makes me think that we have a lurking bug around there, as their actions won't ever run.) In any case, to deal with your immediate need, you must check Rel.Is_Linked, in which case you can regenerate its config, as the first case in this if:

if not Solution.State (Rel.Name).Is_Solved then

I was thinking the other day (in the context of #633) that a cleaner solution would be to have something like Alire.Events with procedures that get called in the proper order at the proper time for all crates/actions. Since that would stall you for a while, I think you can get by with the previous indications, and I can refactor your insertions to the proper places when doing #633.

@Fabien-Chouteau
Copy link
Member Author

I was thinking the other day (in the context of #633) that a cleaner solution would be to have something like Alire.Events with procedures that get called in the proper order at the proper time for all crates/actions. Since that would stall you for a while, I think you can get by with the previous indications, and I can refactor your insertions to the proper places when doing #633.

It would indeed be interesting to have this kind of mechanism.

What about using time-stamps of toml files in the dependency tree for detecting changes?

@mosteo
Copy link
Member

mosteo commented Feb 3, 2021

What about using time-stamps of toml files in the dependency tree for detecting changes?

This is happening already for the root manifest (see e.g. alire/src/alire/alire-roots.ads:99). The places I commented that you can place your hooks shouldn't even get called if the manifest hasn't changed.

The current assumption is that regular dependencies won't be modified by the user, as they're buried in alire/cache/dependencies. In fact, we don't even generate manifest/lockfile for them. This could be changed to allow quick experimentation by modifying the regular dependencies.

In any case, this has to be implemented for linked crates.

doc/catalog-format-spec.md Outdated Show resolved Hide resolved
doc/catalog-format-spec.md Outdated Show resolved Hide resolved
doc/catalog-format-spec.md Outdated Show resolved Hide resolved
doc/catalog-format-spec.md Outdated Show resolved Hide resolved
doc/catalog-format-spec.md Outdated Show resolved Hide resolved
doc/catalog-format-spec.md Outdated Show resolved Hide resolved
This is the possibility for crates to define configuration variables
that will then generate Ada, C, or GPR files based on the values set by
depending crates.

For embedded projects where compile optimization and static memory usage
are important, it is possible to define sizes of cache buffers or
maximum amount of some resources.

It allows to enable or disable features at compile time, such as debug
logging.
@Fabien-Chouteau Fabien-Chouteau marked this pull request as ready for review February 25, 2021 14:04
@Fabien-Chouteau Fabien-Chouteau changed the title [RFC] Start support for crate configuration Support for crate configuration Feb 25, 2021
@Fabien-Chouteau
Copy link
Member Author

@mosteo I think this is ready for review as a first pass.

Things we might want to change is having a top level table called configuration instead of having config_variables and config_settings:

[configuration.variables]
test = {type="Boolean"}

[configuration.settings]
lol.test=42

This will also allow extra fields that are going to be useful in the future:

[configuration]
output_dir = "config"
disabled=false # Disable the configuration feature for this crate
generate_ada=true
generate_c=false
generate_gpr=true

This is the layout I tried when I first started to work on this feature but I couldn't get it to work in the code.

Copy link
Member

@mosteo mosteo left a comment

Choose a reason for hiding this comment

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

I have had a look through the code to get a general idea of the changes, not much to say as I don't get all the details :). I will play a bit with the feature to see if I have any other suggestions, and will have a go at the configuration top-level table, but to me it looks okay.

If you prefer, we can merge as is and I can make my changes on a later PR.

doc/catalog-format-spec.md Show resolved Hide resolved
src/alire/alire-crate_configuration.adb Show resolved Hide resolved
@mosteo mosteo merged commit e17f34a into master Mar 3, 2021
@mosteo mosteo deleted the feat/crate_config branch March 3, 2021 15:18
@mosteo mosteo mentioned this pull request Aug 31, 2021
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