Skip to content

Conversation

@lgarrison
Copy link
Contributor

As requested in #170. With these headers, Gala-EXP builds and runs against just the install directory.

I noticed that some headers, like StringTok.H, appear to be included in public headers, like ParticleReader.H, but not actually used there. In other words, I think we might be able to reduce the footprint of installed headers by moving "internal" includes to the .cc files rather than the .H files.

I just lumped all the new headers into the existing list of expui headers, but I think some of them technically apply to exputil rather than expui. It seemed like there were already headers for both in this list, though.

Also, most of the headers seem to be including each other using system-include style (<> instead of ""), e.g. ParticleReader.H has #include <Particle.H>. System includes will not search the current directory, so this means we need to add two paths to the include path: EXP/install/include and EXP/install/include/EXP. I think the usual style would be to use double-quote includes for your own project's headers; then we could just use the former include path.

@The9Cat
Copy link
Member

The9Cat commented Oct 22, 2025

Thanks. I agree about stuff like StringTok.H. There are probably remnants from a previous refactoring.

Some of the headers in the original commit were also libexpuil rather libexpui as well. We could relocate those to the the exputil dir, but not sure it's worth the effort. What do you think?

I am confused about your last comment however. As written, CMake will install the EXP libraries in <install_root>/include/EXP so one can simply put this path on the build include path for your project. What am I missing here? Alhough, I do agree with you generally about the <> vs "" semantics. Probably all of the project headers should be "". I've had that debate with myself multiple times...

@lgarrison
Copy link
Contributor Author

Some of the headers in the original commit were also libexpuil rather libexpui as well. We could relocate those to the the exputil dir, but not sure it's worth the effort. What do you think?

It's probably fine as-is; I get the sense that expui and exputil are often (always?) used together.

I am confused about your last comment however. As written, CMake will install the EXP libraries in <install_root>/include/EXP so one can simply put this path on the build include path for your project. What am I missing here?

It's that the dependencies (highfive and yaml-cpp) also have directories in include/. EXP (and Gala) include them via #include <highfive/highfive.hpp>, so the include root needs to be at include/, and not include/EXP/. One could add all the individual directories and use #include <highfive.hpp>, but I think the former is good style.

@The9Cat
Copy link
Member

The9Cat commented Oct 23, 2025

Got it. It would be trivial to put the EXP header install at include rather than include/EXP. That's controlled by the CMake FILE_SET destination which I set to include/EXP, perhaps capriciously. Better to have that be include?

@lgarrison
Copy link
Contributor Author

I would actually keep it in include/EXP and change EXP's internal include style to use double quotes. But it's mostly a matter of preference and whether you want downstream users to write #include <EXP/exputil.h> or #include <exputil.h>. I usually prefer the former for its namespacing.

@The9Cat
Copy link
Member

The9Cat commented Oct 23, 2025

All good points. I appreciate the thoughts.

Wrote a quick Python script to do this for every ".H" and ".h" file in the tree (excluding the extern submodule tree and cuda stuff for now). Seems to work in the sense that the code still compiles without issue and I ran the standard (albeit not extensive) unit tests. [We should develop more unit tests...but not anywhere near the top of my priority list.]

See new commit. Maybe try this for your Gala+EXP build to see if the #include <EXP/file.H> strategy works for you?

@The9Cat
Copy link
Member

The9Cat commented Oct 23, 2025

Oh, I can't push this commit to your repo! Doh. I'll cherry-pick back to the originating PR.

@lgarrison
Copy link
Contributor Author

You should be able to push to this branch in my repo, "Allow edits by maintainers" is checked! Upstream is fine too.

@The9Cat
Copy link
Member

The9Cat commented Oct 23, 2025

Huh. Maybe my error somewhere? Couldn't authenticate in the usual way for some reason. Anyway, please merge upstream from getAccelVector.

@lgarrison lgarrison force-pushed the more-install-headers branch from 6024be4 to a69518f Compare October 23, 2025 15:28
@lgarrison
Copy link
Contributor Author

That's perfect; I can confirm that Gala+EXP builds and runs with just the include/ (not include/EXP/) path. Thanks!

@The9Cat
Copy link
Member

The9Cat commented Oct 23, 2025

Okay, let's merge this.

@The9Cat The9Cat merged commit e13260c into EXP-code:getAccelVector Oct 23, 2025
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.

2 participants