-
Notifications
You must be signed in to change notification settings - Fork 108
Read fpm toml #47
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
Read fpm toml #47
Conversation
I've got it reading the first setting from an |
I think this looks good. This implements explicit layout, which we should support also. Based on our past discussion, by default we would encourage users to use the implicit layout, so they just need to put a file in the |
[executables.executable-name] | ||
main = "Main.f90" | ||
source-dirs = "app" | ||
linker-options = ["-O3"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a compiler option, no? Probably not consequential because this is meant as example metadata. I don't know if -Ox flags can be passed to a linker or if they are just ignored. Is there a more meaningful option we can use here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, based on my comment above, I suggest we first tackle the common case of not specifying such options at all, but have a sane default for Debug / Release for all compilers. Exactly as Cargo works. Only later also allow to explicitly set this somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linkers can do some amount of optimization at link time. It's the only way to do inlining of functions from separate compilation units. This is just an example anyway, and this PR doesn't read it anyway.
[tests.test-name] | ||
main = "Spec.f90" | ||
source-dirs = "test" | ||
linker-options = ["-Og"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I only had a minor question.
Implicit layout will be a bit trickier to implement I think. The default would be only one executable, with main having the default expected name (probably All doable. I think we should start with explicit, make sure that works, then come up with lots of example projects, leaving various things out to allow for defaults. |
Here is our last iteration that we did at the Committee meeting in Vegas: And to have more executables in the app directory, you just add more subdirectories just like Cargo does it I think. |
I think the purpose of going for an implicit, minimal, sane default is that it can be implemented most quickly. You can then assess what's most needed to generalize next. I prefer this approach to development and we use it at Cloudrun. Otherwise, we have to make a lot of assumptions on what are all the options and edge cases that will be desired, and we'd spend a lot of time doing that. |
With the toml parser I'm using, it's actually easiest to define the data structure that holds all the necessary settings, and require they be present in the toml file. Then we can figure out how to make that setting optional (have a default). Yes we should start with the simplest possible project and the simplest settings, but it actually is easier to make them required first. |
I'm going to merge this in and then take a stab at simplifying the example project and it's toml file to the bare minimum. |
Start reading the settings for a package from fpm.toml