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

#include "config.h" in WireCellUtil/Exceptions.h #200

Closed
HaiwangYu opened this issue Mar 7, 2023 · 4 comments
Closed

#include "config.h" in WireCellUtil/Exceptions.h #200

HaiwangYu opened this issue Mar 7, 2023 · 4 comments
Assignees

Comments

@HaiwangYu
Copy link
Member

@brettviren I believe this will cause trouble for external source code trying to #include WireCellUtil/Exceptions.h
https://github.com/WireCell/wire-cell-toolkit/blob/master/util/inc/WireCellUtil/Exceptions.h#L27

For example:
Screenshot 2023-03-07 at 4 40 02 PM

Also I don't think we want to install this "config.h", at least not at top level, since the name is so general, right?

@brettviren
Copy link
Member

brettviren commented Mar 8, 2023

Ah, yes this will not work as-is.

We control the config.h file name (in wcb.py) so can pick a less generic name. I think we can also arrange to have it installed so users of WCT will pick it up.

I'll work on this. Mostly for my own checklist, here are my expected steps:

  • update waf-tools to make wcb.py write config.h to a less generic name
  • update waf-tools to install the new header
  • make sure new name can be included either under build/ where it is written and where it is installed
  • update any #include
  • port changes to check-test branch which moves waf-tools to the waft/ subdir. (not needed for master but needed when that branch is merged)

Thanks for finding this mistake!

brettviren added a commit to WireCell/waf-tools that referenced this issue Mar 8, 2023
…toolkit#200

Also fix badly formed variable name for holding path to root-config program
@brettviren
Copy link
Member

Okay, i think i cleaned this up and pushed to master.

I added a bats test which does a simple "external" compilation to check this fix. It won't get run automatically until we merge in #199

@HaiwangYu
Copy link
Member Author

Thanks a lot! Looks like a perfect fix to me. Will test it out by building larwirecell.

@brettviren
Copy link
Member

The config.h was still getting #include ed.

This was partly missed because the old build/config.h was still hanging around. When updating the code in a previous build area, one likely needs to do wcb clean.

A fix is inbound.

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

No branches or pull requests

2 participants