#552 Added pfunit tool and tests.#562
Conversation
Joerg Henrichs (hiker)
added
enhancement
|
There is a certain "asymmetry": this class has a function for getting the include flags, but not the libraries. I would welcome some feedback here. Ideally, we should have a kind of 'dependency' manager, which handles linker AND compiler flags. So far, we only have library flags (in the linker). That would be a bit of work, time we all might not have atm. The consistent solution would be to leave it to the site-specific setup to add the include flags require for pfunit, and I am happy to do that. In lfric, I have a mixin class which automatically adds the include path from pfunit, so it removes the need to add the include flags in all site-specific files (and if a site need something else, they can just add their own pfunit implementation). So, it basically means no need to change any site-specific setup files. But admittedly, it is inconsistent. Not sure what's the best way forward here :) But it is working with skeleton now, so marking this as ready for review. |
Joerg Henrichs (hiker)
added
Ready for review
Joerg Henrichs (hiker)
added
the
Priority
Matthew Hambley (MatthewHambley)
left a comment
Matthew Hambley (MatthewHambley)
left a comment
There was a problem hiding this comment.
A question and some food for thought.
| AR = auto() | ||
| RSYNC = auto() | ||
| SHELL = auto() | ||
| PFUNIT = auto() |
There was a problem hiding this comment.
Now that we have your change to categories on trunk, do you wont to reconsider your position on using them here?
There was a problem hiding this comment.
Yes, I've brought this up-to-date with what is done for the other tools.
| """ | ||
|
|
||
| def __init__(self): | ||
| pfunit_home = os.environ.get("PFUNIT", "") |
There was a problem hiding this comment.
In the future we might provide an argument on this constructor to specify the location of the driver source (that's what we need PFUNIT for). We could then move the use of the environment variable to the top level build script, or not. But I don't think it's worth making that change how.
There was a problem hiding this comment.
I am happy to wait (or if you have details what you want, I can add this in, doesn't sound too complicated).
2 participants
As discussed, this adds the pfunit tool to Fab.
Note that I have NOT based this on #311 (PR #553). IMHO, the latter is kind of optional. Once we have pFUnit, we don't need really need #311 (it's just a nice to have for the future ;) ).
I am setting this as draft for now, to make sure that the core-fab-with-pfunit branch works as expected.