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

FetchContent example #3

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

stigrj
Copy link
Contributor

@stigrj stigrj commented Apr 8, 2019

Copied the pi example from externalproject to fetchcontent.

This version does not treat the current project as an external, as is done in the externalproject, not sure if this is wanted or not.

@bast
Copy link
Member

bast commented Apr 10, 2019

ExternalProject allows better variable scope separation, FWIW. FetchContent makes it easier to access targets. So I think this is a good move. I am a bit confused about naming of directories (fetchcontent is for me an implementational detail so why is this the name of the directory?) but I recommend we merge and then I can suggest changes iteratively from there with follow-up PRs.

@stigrj
Copy link
Contributor Author

stigrj commented Apr 10, 2019

The externalproject and fetchcontent directories are two different cmake setups of the same example code. I guess the idea was to show how both cmake strategies work?

@bast
Copy link
Member

bast commented Apr 10, 2019

Ah OK sorry, then it makes a lot of sense.

@bast
Copy link
Member

bast commented Apr 10, 2019

Can we avoid the configuration of the source file and rather pass the information as command line argument to the binary?

This is not a merge blocker though. This is personal taste. Motivation is that *.in source files are often not correctly highlighted and people editing them may need to understand CMake.

@robertodr
Copy link
Contributor

I second Radovan's suggestion about passing executable as argument.

@bast
Copy link
Member

bast commented Apr 10, 2019

Also gives more flexibility to try out stuff without reconfiguring/rebuilding.

@stigrj
Copy link
Contributor Author

stigrj commented Apr 11, 2019

So if I have a sample input file in

example/pi.inp

I would run as something like this?

$ ../build/bin/pi -x ../build/bin/pi.x -t ../src/template.yml pi.inp

I guess the template file should also be shipped along with the executables when building and installing? If so, where to?

@bast
Copy link
Member

bast commented Apr 11, 2019

What is the difference between bin/pi and bin/pi.x?

I guess the template file should also be shipped along with the executables when building and installing? If so, where to?

Yes, I think so. Where to I am not sure yet. Either a subdirectory or perhaps "datadir" of https://www.gnu.org/prep/standards/html_node/Directory-Variables.html

@stigrj
Copy link
Contributor Author

stigrj commented Apr 11, 2019

pi.x is the c++ executable that accepts a JSON input. pi is a python script that runs parselglossy on the user input and calls pi.x with the processed JSON.

If we anyway pass the executable and template as arguments it would be easier to drop the python script and just use the parselglossy CLI instead. Perhaps this was the intention?

@bast
Copy link
Member

bast commented Apr 11, 2019

To me this suggestion sounds good. @robertodr do we see this correctly?

@robertodr
Copy link
Contributor

So it will be:

parselglossy ...
pi.x input.json

right?

I like it, for this simple example. For more complicated programs (like MRChem), the front-end script would do more than just the parsing, but that's beyond the scope of this tutorial repo.

@stigrj
Copy link
Contributor Author

stigrj commented Apr 12, 2019

@bast can you have a look at the fetchcontent/tests/runtest_config.py? I now run parselglossy as a subprocess in configure. The actual integration test is the C++ executable with parsed JSON input.

Any suggestion for fixing the template path in this file?

@stigrj
Copy link
Contributor Author

stigrj commented Apr 12, 2019

It's might be better to run parselglossy in the individual test scripts for each integration test, before the call to runtest.run?

@bast
Copy link
Member

bast commented Apr 12, 2019

I'll have a look and report back ...

@stigrj
Copy link
Contributor Author

stigrj commented Apr 12, 2019

In the latest commit I moved the parselglossy parse up one level to the test scripts, but that required a manual copy of the input file to the binary test dir. Not sure which of the last two commits is preferred?

@bast
Copy link
Member

bast commented Apr 12, 2019

Now looking at it ...

@bast
Copy link
Member

bast commented Apr 12, 2019

Running the parselglossy step in the test script is more explicit but the question is whether the tests are supposed to mirror tests in production codes which will probably abstract the parselglossy step and hide it inside the launcher. So the question is how visible you want it to be.

So the question is for me:

  • are the tests mirroring a real-life situation for end-users? in this case we should perhaps abstract it out.
  • or are the tests documenting how to interface codes for developers? then explicit may be better than implicit.

The '../../../src/template.yml' could be solved with a symlink.

@stigrj
Copy link
Contributor Author

stigrj commented Apr 12, 2019

If we parse in the test script we can loop over different grammars and perform the same test for getkw, dalton, etc syntaxes. Do we support any other grammars atm?

@stigrj
Copy link
Contributor Author

stigrj commented Apr 12, 2019

Now passing template.yml around to share/ dirs

@stigrj stigrj force-pushed the fetchcontent-example branch from d0972a1 to 7738c75 Compare April 12, 2019 14:57
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.

3 participants