-
Notifications
You must be signed in to change notification settings - Fork 46
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
Migrates and briefly documents the UBet analysis code for public reference in the LangSec slides. #6552
Conversation
0658e67
to
7fd0e28
Compare
…parent dockerfile is now different, as also in #6551
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.
Great job adding all this documentation! I've made a couple of notes that we could consider. Also, I'm thinking we might want to synch with a couple of other branches going into main.
…want (and so that I can use it when I need it later)
… for analysis if desired
|
||
RUN cp modules/c++/nitf/show_nitf++ nitro_Release | ||
|
||
RUN polytracker instrument-targets \ |
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.
We will need to add --cflog
once #6553 is merged. Additional flag needed to not change existing behaviour.
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.
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.
We need to add it here as the Dockerfile.nitro
does not exist on #6553. Either we do it now, or we add it as a follow-up PR. We could also merge all our changes into a separate branch and then make the final changes there. But, I think merging #6553 first, then just adding the --cflog
here and merging makes the most sense to me. Ideally this wouldn't be dependent on #6553 to be merged, in practice it is hard to avoid.
|
||
RUN polytracker build cmake --build . -j$((`nproc`+1)) --clean-first --target show_nitf++ --config Debug | ||
RUN cp modules/c++/nitf/show_nitf++ nitro_Debug | ||
RUN polytracker instrument-targets \ |
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.
We will need to add --cflog
once #6553 is merged. Additional flag needed to not change existing behaviour.
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.
as above 🙂
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.
LGTM! We need an additional change for it to synch with the instrumentation branch, but we can follow up with that or add an additional commit depending on order of merge.
|
||
RUN pip install cxxfilt | ||
|
||
RUN git clone https://github.com/mdaus/nitro.git |
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.
I suggest we pin to the specific version of Nitro that we know works, just in case something upstream breaks our instrumentation later on.
This commit has I chose is from mid-March, but I didn't test it. @kaoudis do you think we need an earlier version?
RUN git clone https://github.com/mdaus/nitro.git | |
RUN git clone https://github.com/mdaus/nitro.git | |
WORKDIR /polytracker/the_klondike/nitro | |
RUN git checkout 342f552768e249e86df702062ff3f60ea1ec813a |
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.
I think committing a version that matches the version when we were working on this is probably best for now until I/we get a chance to add more analysis stuff here and start working on this again!
Looks like some recent Nitro commits are just merging changes from coda-oss, which may perhaps be used more inside Maxar (Nitro is the only open source use I am currently aware of) and coda-oss does seem to introduce a good chunk of Nitro's variability. I think pinning to b39ccc4c07e84e6c05cecb9ae24143373a3ed8e2 could be better since that would be the one that we were looking at when we did the work. What do you think?
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.
Yes! Let's pin the version you are suggesting @kaoudis! That's the one we have a working result from.
…eeds things up - follow the build job
…s to complete instead of hanging forever for right this second
❗ caution: depends on #6553 and will not accurately reproduce our results without merging that code first / too.