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

Adding epicsArchVerify to epicsArchChecker #219

Merged
merged 6 commits into from
Nov 15, 2024

Conversation

KaushikMalapati
Copy link
Contributor

Description

Adding a switch to run epicsArchVerify in epicsArchChecker, and running it by default for lcls2 hutches.

Motivation and Context

https://jira.slac.stanford.edu/browse/ECS-6632

How Has This Been Tested?

Interactively

Where Has This Been Documented?

Updated readme and file comments

@KaushikMalapati KaushikMalapati changed the title Eac lcl2 Adding epicsArchVerify to epicsArchChecker Nov 14, 2024
README.md Outdated Show resolved Hide resolved
scripts/epicsArchChecker Show resolved Hide resolved
@KaushikMalapati KaushikMalapati requested review from tangkong and silkenelson and removed request for ZLLentz November 15, 2024 07:09
silkenelson
silkenelson previously approved these changes Nov 15, 2024
Copy link
Collaborator

@silkenelson silkenelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me now and I'll refer to @tangkong et al for a more technical review

Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the JIra ticket, I thought the intended behavior was:

if LCLS2_HUTCH:
    run epicsArchVerify
elif LCLS1_HUTCH:
    run epicsArchChecker

It seems that this runs both for LCLS2Hutches. The above is what Chris O'Grady suggested in the ticket. Have we confirmed that we don't miss anything by running just epicsArchVerify?

scripts/epicsArchChecker Outdated Show resolved Hide resolved
scripts/epicsArchChecker Show resolved Hide resolved
@silkenelson
Copy link
Collaborator

@tangkong thank you for the quick check: Kaushik did check if epicsArchVerify finds everything that epicsArchChecker does and it does not, so we can't easily do as originally suggested. For now, we are going with 'this is better' and will deal with the duplication of checks in a future release, mainly as we'd need to decide where the full check is supposed to live (engineering_tools or the DAQ release) and opinions on the correct solution may differ.

@KaushikMalapati KaushikMalapati merged commit c281cb3 into pcdshub:master Nov 15, 2024
2 checks passed
@KaushikMalapati KaushikMalapati deleted the eac-lcl2 branch November 16, 2024 00:03
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.

3 participants