Skip to content

Conversation

@LKedward
Copy link
Member

Set of changes to address #164 as discussed there.

  • Removes filtering out of executables/tests not specified in the manifest; this allows all programs found in app/ and test/ directories (and subdirectories) into the package model.

  • Does not affect existing behaviour of [[executable]] and [[test]] entries in manifest:

    • Can still specify non-default layouts with these entries;
    • Can override individual settings for auto-discovered executables.
  • Adds a 'scope' to each source, based on source file location, to control which modules can be used within:

    • Library modules (from src/) can only use other library modules or modules from dependencies;
    • Programs and modules in app/ and test/ can use library modules and any modules defined in the same directory as the executable/test;
    • Library modules cannot use modules in app/ or test/;
    • Module dependency resolution fails fatally if a source file cannot be found for a use statement that satisfies these scoping rules.
  • Adds a new example demonstrating:

    • Automatic discovery of programs/tests;
    • Overriding of a discovered executable name in manifest;
    • Modules in same directory as apps and tests.

@awvwgk
Copy link
Member

awvwgk commented Sep 26, 2020

This looks like a useful addition, therefore apologies for the next question: How do you turn it off?

Cargo can disable those by: https://doc.rust-lang.org/cargo/reference/cargo-targets.html#target-auto-discovery

@LKedward
Copy link
Member Author

How do you turn it off?

Fair question. I haven't implemented a way to disable it yet but agree that it is needed.
What manifest syntax would you suggest? Would you prefer this to be included in this PR or a separate one?

@awvwgk
Copy link
Member

awvwgk commented Sep 26, 2020

What manifest syntax would you suggest?

In Cargo it is top-level (meaning [package] which is top-level in fpm).
But maybe a separate section in the fpm.toml is preferable:

[build]  # or config, ...
auto-tests = false
auto-executables = false

Naming would be important, since this is going to be the section holding all the build configuration rules later, I guess.

Would you prefer this to be included in this PR or a separate one?

The syntax will need a bit of discussion first, might be worth deferring than.

Edit: Let's discuss this in a separate issue: #191

@urbanjost
Copy link
Contributor

]So if you have multiple test programs that all shared a module either in the test directory or specified by [test.dependencies]
that you did not need to be used by anything but the test programs (a common case for me where I have a module essentially just used for unit tests)? It sounds like I would not have to put the programs in seperate subdirectories and that I could put everything directly in test? If I wanted to have each test in a subdirectory would I have to have the test module in each subdirectory as well? I am being lazy in that I could look at the code but I am not positive in the discussions whether the term "directory" is being used to mean the top directories app/, test/ and src/ or the subdirectories currently required in h-fpm for each executable.

@LKedward
Copy link
Member Author

@urbanjost, yes your understanding is correct: in this PR you no longer need separate subdirectories in app/ or test/ for multiple executables and tests and you no longer need to specify each test and executable in fpm.toml. You can still use subdirectories but as you point out you can't share modules easily between executables in different subdirectories, so in this case I would recommend using [test.dependencies] instead.

@certik
Copy link
Member

certik commented Sep 28, 2020

This looks awesome. Why don't you implement the following:

[build]
auto-tests = false
auto-executables = false

And add a test for it.

Then this PR will be ready to merge. In the meantime, we will come up with some way to move forward in #191, and if we end up with different names, it's easy to rename in this PR.

Returns canonical path form with redundant artifacts.
Add test on path of program source with source-dir of [[executable]] entry.
For case of two executables with same name in different directories, both with overrides in fpm.toml
Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

Looks good so far.

For the build_config I would suggest to have the toml-f build interface handle the construction of the data structure. The idea of tomlf_build is to allow querying an existing data structure, while adding omitted entries. Therefore, after the parsing is complete the data structure could be serialized to contain all the internal defaults for inspection or rerunning.

I implemented separate default initializers for library and executables since they depend on the “environment,” like present directories, which would have been annoying to handle in the test suite. For the build_config this doesn't seem to required. See comments below.

Contains an app and a test that should be ignored by auto-discovery - this is checked in the CI scripts.
No need for separate default initializer for build table.

Co-authored-by: Sebastian Ehlert <[email protected]>
@LKedward
Copy link
Member Author

Thanks for the explanation @awvwgk, that makes sense. Changes applied.

@awvwgk
Copy link
Member

awvwgk commented Sep 29, 2020

Nice work on the batch commit for suggested changes. Sorry for pointing out now that the build_config doesn't have to be allocatable anymore.

Copy link
Member

@certik certik left a comment

Choose a reason for hiding this comment

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

I am fine to merge as is.

I think we can merge this, and we we come up with different names in fpm.toml in #191, we can send another PR to change that.

Copy link
Member

@milancurcic milancurcic left a comment

Choose a reason for hiding this comment

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

Looks great!

@milancurcic milancurcic merged commit 75c1da5 into fortran-lang:master Oct 2, 2020
@urbanjost
Copy link
Contributor

was going to ask about a use for an intrinsic module, but guess it is just easier to try it now.

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.

Automatically discover executables & tests

5 participants