-
Notifications
You must be signed in to change notification settings - Fork 184
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
Add conifer external #8780
Add conifer external #8780
Conversation
A new Pull Request was created by @thesps (Sioni Summers) for branch IB/CMSSW_13_3_X/master. @smuzaffar, @aandvalenzuela, @iarspider, @cmsbuild can you please review it and eventually sign? Thanks. |
Yes |
Pull request #8780 was updated. |
Thanks, done. |
@cmsbuild please test |
<tool name="conifer" version="@TOOL_VERSION@"> | ||
<client> | ||
<environment name="CONIFER_BASE" default="@TOOL_ROOT@"/> | ||
<environment name="INCLUDE" default="$CONIFER_BASE/conifer/backends/cpp/include"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please change $CONIFER_BASE/conifer/backends/cpp/include
to $CONIFER_BASE/include
( as you copied the header in %{i}/include
).
Also please add <use name="json"/>
after </client>
Shouldn't this PR also delete /L1Trigger/Phase2L1ParticleFlow/interface/conifer.h and migrate the small amount of PF code that refers to this? Or will that be done in a separate PR? |
@thesps , please also include |
@tomalin , not this PR but yes there should be a cmssw PR to delete L1Trigger/Phase2L1ParticleFlow/interface/conifer.h and start using |
… to Requires of cmssw-tool-conf
Pull request #8780 was updated. |
please test (just to make sure external is built properly) |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c326f2/35358/summary.html Comparison SummarySummary:
|
+externals This looks good to go in. @thesps , is cmssw PR ready to make use of this change? |
This pull request is fully signed and it will be integrated in one of the next IB/CMSSW_13_3_X/master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @sextonkennedy, @antoniovilela, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
I see cms-sw/cmssw#42663 waiting for new external. |
Okay now having tried this from the IB last night
At least we picked up the external okay... |
yes, it should be fixed upstream in |
I'll do this ASAP (but probably in about 2 hours from now due to other commitments). Then, after fixing and updating here, I would like to use the |
In your cmssw dev area (based on one of the latest IBs), you just need to update |
This PR adds a new external
conifer
that is to be used for emulation of BDTs in the L1T. We just need to pull in a single header file (this one).This is my first time adding an external, so to check: the header file itself does
#include "nlohmann/json.hpp"
, which is already an external. Do I need to add a requirement onjson
?