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

Jinja2 expansion xml files should go to CMAKE_CURRENT_BINARY_DIR #54

Closed
wdconinc opened this issue Aug 12, 2022 · 6 comments · Fixed by #672
Closed

Jinja2 expansion xml files should go to CMAKE_CURRENT_BINARY_DIR #54

wdconinc opened this issue Aug 12, 2022 · 6 comments · Fixed by #672
Labels
topic: infrastructure Regarding build system, CI, CD

Comments

@wdconinc
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Users may make the epic code and end up with an epic.xml file in their source dir. They may do something with this file, which refers to environment variables. That may lead to confusing results.

Describe the solution you'd like
With produced xml files in the build directory, it is less likely that they would be used inadvertently assuming they work from the source dir top level.

Describe alternatives you've considered
Alternative is not changing.

@veprbl
Copy link
Member

veprbl commented Aug 12, 2022

All build products should always go to the build directory. That helps to prevent them from being committed to the repo.

@wdconinc
Copy link
Contributor Author

Totally agree! This was backwards compatibility when we switched from one ecce.xml in the repository to a template with build products. The benchmarks are still using a crazy installation scheme where we are running against a non-installed version in its source directory.

So, yes, and for every rule there's an exception that may or may not have been a good idea at the time ;-)

@wdconinc
Copy link
Contributor Author

@wdconinc
Copy link
Contributor Author

Fixing that issue required having a way to refer to ip6 without using a symlink. You see, it's all related :-)

@c-dilks
Copy link
Member

c-dilks commented Aug 13, 2022

I think this is good idea. Would it have any impact on the typical workflow of changing something in compact/some_detector.xml, and then calling ddsim (i.e., without running cmake)? I suppose if you set $DETECTOR_PATH to the directory that has your compact/some_detector.xml, then it would be fine.

@wdconinc
Copy link
Contributor Author

Yes, I'd suggest that workflow would be easiest resolved by setting DETECTOR_PATH to a custom location.

@veprbl veprbl added the topic: infrastructure Regarding build system, CI, CD label Oct 1, 2022
@wdconinc wdconinc linked a pull request Mar 18, 2024 that will close this issue
8 tasks
github-merge-queue bot pushed a commit that referenced this issue Mar 19, 2024
#672)

### Briefly, what does this PR introduce?
The build system writes the evaluated jinja templates (entrypoint xml
files) into the source directory instead of a build directory. This PR
moves these temporary build artifacts into the build directory instead.

TODO:
- [x] fix common_bench to do a proper install into a prefix instead of a
build into the source directory

### What kind of change does this PR introduce?
- [x] Bug fix (issue #54)
- [ ] New feature (issue #__)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No.

### Does this PR change default behavior?
Yes, no more xml files will be written to the source directory; `make
install` or bust.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: infrastructure Regarding build system, CI, CD
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants