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

Read serviceconfig path from __file__ or exe location and update README. #355

Conversation

DelpireNI
Copy link
Contributor

@DelpireNI DelpireNI commented Aug 16, 2023

What does this Pull Request accomplish?

Updating how we are loading serviceconfig and other files. If we are running as an exe, we will look next to the exe, otherwise we will look next to __file__

Why should this Pull Request be merged?

People building exe's have run into issues referencing their serviceconfig and UI files. The recommendation for UI files was to include it in the exe using --add-path. I was going to recommend this for serviceconfig files as well, but realized this would effectively dual source the serviceconfig as the DiscoveryService expects a copy on disk.

What testing has been done?

Tested running example as both a script and as an exe.

…env, and the requirement to include .serviceconfig file.

Signed-off-by: Chris Delpire <chris.delpire@ni.com>
@github-actions
Copy link

github-actions bot commented Aug 16, 2023

Test Results

     12 files  ±0       12 suites  ±0   1m 57s ⏱️ -1s
   182 tests ±0     157 ✔️ ±0    25 💤 ±0  0 ±0 
2 172 runs  ±0  1 872 ✔️ ±0  300 💤 ±0  0 ±0 

Results for commit be1d866. ± Comparison against base commit 856cb37.

♻️ This comment has been updated with latest results.

@DelpireNI
Copy link
Contributor Author

Moving to draft. My suggestion to include the serviceconfig using --add-data is not sufficient. This will result in dual sourcing the serviceconfig file if the measurement is statically registered. I have filed a bug with a suggested fix. #356

…bute exists.

Signed-off-by: Chris Delpire <chris.delpire@ni.com>
Signed-off-by: Chris Delpire <chris.delpire@ni.com>
Signed-off-by: Chris Delpire <chris.delpire@ni.com>
Signed-off-by: Chris Delpire <chris.delpire@ni.com>
@DelpireNI DelpireNI changed the title Updating README to mention requiring pyinstaller to be installed to v… Read serviceconfig path from __file__ or exe location and update README. Aug 21, 2023
@DelpireNI DelpireNI marked this pull request as ready for review August 21, 2023 20:33
@DelpireNI DelpireNI requested a review from mshafer-NI as a code owner August 21, 2023 20:33
@DelpireNI DelpireNI requested a review from bkeryan August 21, 2023 20:59
Signed-off-by: Chris Delpire <chris.delpire@ni.com>
Signed-off-by: Chris Delpire <chris.delpire@ni.com>
@DelpireNI DelpireNI requested a review from bkeryan August 22, 2023 21:24
@dixonjoel
Copy link
Collaborator

Reminder - run 'poetry run pytest' locally. The state we're in right now before we add a self-hosted test runner is that there are a number of tests that are skipped on the PR checks.

@DelpireNI
Copy link
Contributor Author

Reminder - run 'poetry run pytest' locally. The state we're in right now before we add a self-hosted test runner is that there are a number of tests that are skipped on the PR checks.

Tests passed.

@DelpireNI DelpireNI merged commit 4bb484b into main Aug 24, 2023
@DelpireNI DelpireNI deleted the users/cdelpire/update-readme-to-improve-create-executable-for-python-scripts-section branch August 24, 2023 17:37
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