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 options for ioc main #24

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ZohebShaikh
Copy link

This PR is created in relation to this issue epics-base/pvxs#49

@mdavidsaver
Copy link
Member

CI failures on OSX are unrelated (#25) to this PR.

@OCopping OCopping self-requested a review September 4, 2024 10:33
@OCopping
Copy link
Contributor

OCopping commented Sep 5, 2024

Are you able to squash these commits into only a couple please?

@ZohebShaikh ZohebShaikh force-pushed the adding-options-for-ioc-main branch from 5962b38 to e1284a4 Compare September 5, 2024 09:41
@ZohebShaikh
Copy link
Author

Are you able to squash these commits into only a couple please?

I've gone ahead and squashed the commits into a single one as requested. Please feel free to let me know if there's anything else you'd like me to adjust!

@OCopping
Copy link
Contributor

OCopping commented Sep 5, 2024

CI is broken due to actions/upload-artifact@v2 being deprecated. Fixing in #29

@ZohebShaikh
Copy link
Author

ZohebShaikh commented Oct 2, 2024

@OCopping I noticed that #29 has been merged. Is this PR ready to be merged as well? I'm unable to re-run the CI to verify.

@OCopping
Copy link
Contributor

OCopping commented Oct 2, 2024

@ZohebShaikh can you rebase this on main so we can try the CI again?

@ZohebShaikh ZohebShaikh force-pushed the adding-options-for-ioc-main branch from e1284a4 to 8ac8589 Compare October 2, 2024 08:15
@ZohebShaikh ZohebShaikh force-pushed the adding-options-for-ioc-main branch from 8ac8589 to 7f86378 Compare October 3, 2024 14:19
@ZohebShaikh
Copy link
Author

@OCopping I noticed the CI is passing. Could we proceed with the merge?

Copy link
Collaborator

@coretl coretl left a comment

Choose a reason for hiding this comment

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

@mdavidsaver I'm happy with this, can we merge?

Copy link
Member

@mdavidsaver mdavidsaver left a comment

Choose a reason for hiding this comment

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

To be honest, I'm not enthused about this start_ioc() being supported public API. However, I understand the appeal.

Maybe a less bad option would be adding two arguments pvAccess=True and iocInit=True?

Passing pvAccess=False would skip loading pvAccessIOC. iocInit=False would skip calling iocInit(), which allows the caller of start_ioc() to to load addition dbd, libraries, run arbitrary ioc shell commands, then call iocInit()

@@ -63,7 +64,7 @@ def ioc(cmd):
'''
return iocshCmd(cmd.encode())

def start_ioc(database=None, macros='', dbs=None):
def start_ioc(extra_dbd_load, extra_dso_load, database=None, macros='', dbs=None):
Copy link
Member

Choose a reason for hiding this comment

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

New arguments should be appended, with a default which preserves the current behavior.

@mdavidsaver
Copy link
Member

iocInit=False

... or maybe this becomes a new function prepare_ioc(...)?

@mdavidsaver
Copy link
Member

To be clear, the only change I insist on is reordering of arguments to preserve backwards compatibility.

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.

4 participants