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

Fix #1427, Add default build/install scripts #1428

Closed
wants to merge 1 commit into from

Conversation

gskenned
Copy link

Description
Fixes issue 1427. Added two default build/install scripts.
mk-osal-no-test.sh - Builds OSAL and installs in ../local
mk-osal-test.sh - Builds OSAL, runs OSAL tests, and installs in ../local

Testing performed
Steps taken to test the contribution:

  1. Ran both build scripts successfully.
  2. All OSAL tests in the build passed.

Expected behavior changes
The default build scripts provide example build commands using relative paths.

System(s) tested on

  • Hardware: AWS
  • OS: Ubuntu 20.04 LTS
  • Versions: OSAL 5.0.99

Contributor Info - All information REQUIRED for consideration of pull request
Grafton S. Kennedy, III - Vantage Systems, Inc.

@dzbaker dzbaker linked an issue Nov 20, 2023 that may be closed by this pull request
@gskenned gskenned force-pushed the fix-1427-default-build-scripts branch from 679c2c9 to 807451d Compare November 20, 2023 13:01
@gskenned gskenned changed the title Fix nasa#1427, Add default build/install scripts Fix #1427, Add default build/install scripts Nov 20, 2023
@gskenned
Copy link
Author

gskenned commented Nov 20, 2023

Changed Fix nasa#1427 to Fix #1427 in PR title and commit comment per format checker.

@jphickey
Copy link
Contributor

I'm not sure if we should have these as .sh scripts. Although I think its good information to capture, I get nervous if someone says "just run this mystery script" ...

For one, it is missing the !# at the start, but also it doesn't handle any errors, e.g. if the "mkdir" or "cd" fails, then you'll end up running cmake in the wrong dir. Or something.

Instead of giving a script that is directly in script form -- my recommendation would be to add this to the README, and let the user put it (e.g. copy/paste) into their own script file. This is safer, as it doesn't make assumptions about the user's system, and the onus is on the user to make sure its correct for their system before they go and run it.

@gskenned
Copy link
Author

Thank you for your review comments, Joe. I agree with all of them. This PR is ready to close. Mostly my intention was to document scripts that I shared with another team member.

@dzbaker dzbaker closed this Nov 28, 2023
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.

Add default build/install scripts for regular and test builds.
3 participants