Move "Root Actor is Loaded.vi" to AFPP Shared Library

[neilfenstein]

GitHub Issue for the Pull Request

Issue 50

GitHub Discussions Related to this Pull Request

List any GitHub discussion links related to this PR

Checklists

Completing these checklists ensures that your PR meets our build server requirements on the first attempt. If you're unable to complete any of the following checks, please submit your PR as a draft to the best of your ability. We will provide any clarification you may need after posting it.

• I do not require assistance from NI to complete any of the following checks.
• The changes in this PR are based on the appropriate NI-repo feature branch
• I am submitting the changes in this PR to the appropriate NI-repo feature branch
• I built a VI Package using the Powershell build tool.
• I installed the VI Package produced by the Powershell build tool and tested my change.
• I tested my changes after installing the VI package.
• NI has my contributor license agreement.

Summary of Changes

Moved Root Actor is Loaded.vi from Message Maker Library to AFPP Shared Library.

Reason for Change

Reduction of Library linkage.

Visual Aids

Dependency linkage before work

Dependency linkage after work

Updated Icon and Library Namespace visual

Location in AFPP Shared Library

Additional Information

Include any further details that may assist the reviewer in understanding the context of this PR.

Testing

This section describes the automated and manual tests performed for this bugfix/feature.

Manual Tests

Created new project and added the vi before moved and repeated after moved as shown in the screen shots above.

[svelderrainruiz]

@neilfenstein can you please sign your commits? This is a blog post from SAS Workshops that explains how to do that.

[neilfenstein]

I had to update my code with an alias file change and the .tgitignore file to get a sign commit, let me know if this is OK, if not I can re-fork and commit the changes signed.

[svelderrainruiz]

i haven't lost track of this @neilfenstein . AF has foundational CI only and no release pipeline, i am planning on introducing it very soon.

[niACS]

@neilfenstein Did you run the manual test plan for the providers, here: https://github.com/ni/actor-framework/blob/develop/Documentation/Manual%20Test%20Scripts/Manual%20Test%20Script%20for%20Actor%20Framework%20Project%20Providers.docx

[niACS]

I have manually inspected the provider libraries to confirm these changes, and I have run the manual test script for the providers.

All work was performed in LabVIEW 2020 20.0.1f1.

[shivaprasad-basavaraj]

I have manually inspected the provider libraries to confirm these changes, and I have run the manual test script for the providers.

All work was performed in LabVIEW 2020 20.0.1f1.

@niACS @neilfenstein There are a few issues with this PR which is preventing me from accepting it and merge the changes up.

• The commits in the PR are not signed which is resulting in the merge being blocked. While I can bypass this requirement, I would rather have this fixed properly by signing the commits.
• The PR has other changes apart from just the changes to move the VI mentioned in the PR description as shown in the screenshot below. These are problematic for example there are changes in Core/Actor Framework 2024 for 2020.vipb which would be incorrect since there have been multiple updates to this file since he raised his PR. There are also new pipeline yml's. Readme etc.. This PR should contain only the move of the VI and any lvlib's etc which are directly affected by it.

• This PR also updates the VIPM packages in the Builds folder of the repo which is supposed to contain the most recent VIPM package published from the repo. Since the PR is very old the VIPM package is completely out of date. I can create another PR once this goes in which would update the VIPM package built using the updated code in develop but I would rather have the correct VIPM packages published in the first place.

• The PR is targeted to a feature branch and not the develop branch. I would rather have this PR targeted directly to the develop branch to avoid another merge from the feature branch into develop.

Given all these issues I think the best course of action would be to create a new PR with the above concerns addressed.

[neilfenstein]

Hi,
Excuse the delay in getting back to you it has been a busy few weeks personally & professionally.

As you can see from my commit dates it's been about a year since the original tasks were undertaken.
In that time a lot has changed and at the time I was a bit early in the adoption so there was a lot of uncertainty of process and missing documentation.

Initial commits were not signed, there was not any discussion of this and the documentation was referencing icon editor etc. Very confusing. I signed the branch after the fact. In accordance with my research it is as good as signing each commit as each SHA1 signing is endorsing the work up to that commit.
Subsequently, to test the PR I had to build the packages, but found that some type specialisation structures were causing the builds to fail. It was agreed that I should remove the structures. Which I did a couple of months ago.

So to clarify I have made 4 commits to resolve the PR

• 2 initial code edits where I moved the cyclic dependancies and tidied up icons/refs.
• Another to sign the previous work.
• A final commit to remove the type specialisations structures enabling the build.

In the mean time Sergio has done some work on my fork to look in to the CI pipeline, I had no part in this and it looks like from the commit history that he has change Yaml and the vipbs etc. he has also merged this in to my branch.

You have suggested starting a new PR, if this is the best way to solve this, perhaps you can state clearly what I should do and when and how to do it to finalise this.

I appreciate the need to get this right but I have done the work several times due to poor documentation and lack of process clarity, hopefully 1 more time gets it complete 🤞
If you would like me to make notes and cleanly update the contribution documentation to save others any future problems then let me know.

[shivaprasad-basavaraj]

@neilfenstein Apologies for the delay in getting back to you on this PR. I was out parts of last week and this comment on the PR slipped through.

Unfortunately with the PR being so old there is really no clean way in taking this forward apart from starting afresh in a new branch in your fork. Make sure you pull the latest code from the actorframework repo into your fork and create a branch where you can make the required changes. From what I can see in the commit history of this PR, the crux of the required changes were in these commits (020930b and 7c8a512). You can redo these changes in the new branch and raise a new PR in GitHub.

[neilfenstein]

Hi @shivaprasad-basavaraj,
To confirm process,

1. update to latest from NI Develop branch.
2. Create new Branch (any name preferences?)
3. Update to address task fixes.
4. Request Merge. This should be against the NI develop branch, yes?

Is that expected workflow?

Thanks for your help.
I was early in the process and it was my first use of a GitHub and Forking, so I assumed the process was 2 stage to get the NI repo branch to match my Fork, then to Merge to develop.

Neil.

[shivaprasad-basavaraj]

Hi @shivaprasad-basavaraj, To confirm process,

1. update to latest from NI Develop branch.
2. Create new Branch (any name preferences?)
3. Update to address task fixes.
4. Request Merge. This should be against the NI develop branch, yes?

Is that expected workflow?

Thanks for your help. I was early in the process and it was my first use of a GitHub and Forking, so I assumed the process was 2 stage to get the NI repo branch to match my Fork, then to Merge to develop.

Neil.

Yes. That's correct.

• Update your fork's develop to latest develop from actor framework.
• Create a branch from that your develop branch
• Make the changes
• Create a PR with target set to ni:develop.