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

For built exes, copy the entire containing directory for PPL UIs #257

Merged
merged 4 commits into from
Aug 11, 2023

Conversation

pbirkhol-ni
Copy link
Contributor

What does this Pull Request accomplish?

I received some offline feedback for #255. It didn't handle the case where the UI PPL had external dll dependencies. This change will copy the entire containing PPL directory during the build so that any necessary dlls also get copied. It also updates the Keysight example and documentation to mention that PPLs should be placed in a subdirectory next to the Measurement Logic.vi.

Why should this Pull Request be merged?

We don't expect UIs with dll dependencies will be common, but it is certainly possible and we should support it.

What testing has been done?

I created a measurement with a UI that uses the LabVIEW keyboard VIs, which has a dependency on lvinput.dll. I verified that before my change, the UI wouldn't load in InstrumentStudio. After the change, the dll gets copied and the UI loads.

@dixonjoel
Copy link
Collaborator

Please update the PR title before submitting for accuracy when looking back at PRs.

@pbirkhol-ni
Copy link
Contributor Author

Please update the PR title before submitting for accuracy when looking back at PRs.

Ugh. Why is github like this?

@pbirkhol-ni pbirkhol-ni changed the title Users/pbirkhol/copy entire ppl dir For built exes, copy the entire containing directory for PPL UIs Aug 10, 2023
@bkeryan
Copy link
Collaborator

bkeryan commented Aug 10, 2023

Please update the PR title before submitting for accuracy when looking back at PRs.

Ugh. Why is github like this?

The default behavior probably makes more sense for forks that don't have users/foo/ boilerplate.

FYI, there is an action to run checks on the PR title: https://github.com/marketplace/actions/pr-title-checker

Copy link
Contributor

@jasonmreding jasonmreding left a comment

Choose a reason for hiding this comment

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

The exe buildspec is still pulling in Measurement UI.vi which isn't necessary. This is something that we should fix across all of our examples and the generator. If you want to address this in a follow on PR with a more comprehensive set of changes, I'm fine with that.

@pbirkhol-ni
Copy link
Contributor Author

The exe buildspec is still pulling in Measurement UI.vi which isn't necessary. This is something that we should fix across all of our examples and the generator. If you want to address this in a follow on PR with a more comprehensive set of changes, I'm fine with that.

Sure. I'll do a quick follow-on PR.

@pbirkhol-ni pbirkhol-ni merged commit e55471c into main Aug 11, 2023
1 check passed
@pbirkhol-ni pbirkhol-ni deleted the users/pbirkhol/copy-entire-ppl-dir branch August 11, 2023 15:43
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