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

Add config check automation tool for wolfSSL configure #5876

Closed
wants to merge 1 commit into from

Conversation

gojimmypi
Copy link
Contributor

@gojimmypi gojimmypi commented Dec 11, 2022

Description

The wolfSSL ./configure command has an extraordinary number of options and parameters that select target platforms, fine tune installation, optimize performance, select various features, and more. Managing and experimenting with the options can be a daunting experience, particularly for the new user.

This tool allows options to be specified in a more readable command file with comments. The output of the refresh script breaks out the resultant enabled and disabled features into more easily viewed summary files.

See the README.md for full details.

Fixes zd# n/a

Testing

How did you test?

There's no change to the underlying wolfSSL library. This is only a configuration assistance tool.

Run the from the config_check directory:

cd ./scripts/config_check
./refresh.sh

Observe files created in the config_check directory.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@dgarske
Copy link
Contributor

dgarske commented Dec 12, 2022

@jpbland1 is going to review and give this a try.

@kaleb-himes
Copy link
Contributor

retest this please.

The multi-line `./configure` command with all parameters on subsequent lines is stripped of comments and everything placed on a single line statement when executed.

Upon execution, the entire output is set to a file called [output.txt](./output.txt). Additionally, the enabled/disabled features (those items with an asterisk in the output and the word "yes" or "no")
are separated and stored in the respective [Enabled-Features.txt](Enabled-Features.txt) and [Enabled-Features.txt](Enabled-Features.txt) files.
Copy link
Contributor

Choose a reason for hiding this comment

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

should be Disabled-Features.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. updated.

config_check/refresh.sh Outdated Show resolved Hide resolved
echo ""

# get the contents of the command file, trimming all text after the # character
WOLFSSL_CMD="$(cat $WOLFSSL_CMD_FILE | cut -d'#' -f1)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is error checking important here? when I comment out ./configure in cmd.txt it doesn't work but it can't really do any damage and just leads to my terminal telling me command not found followed by all the text below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some error checking and comments in the cmd.txt file. There's not much difference other than not doing the file operations after the ./configure command.

Copy link
Contributor

@jpbland1 jpbland1 left a comment

Choose a reason for hiding this comment

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

made some comments, tested it and it works on my setup

@gojimmypi
Copy link
Contributor Author

Heads up I added the include.am to Makefile.am.

I also don't know why I was previously moving the the options.h. I changed that to a copy.

Copy link
Contributor

@jpbland1 jpbland1 left a comment

Choose a reason for hiding this comment

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

looks good, refresh is executable and an error message prints out when I remove configure from cmd.txt

@gojimmypi
Copy link
Contributor Author

jenkins retest this please

@jpbland1
Copy link
Contributor

github says this branch is behind master, maybe merge the new changes and see if that fixes the build problem

@dgarske dgarske assigned dgarske and unassigned gojimmypi and jpbland1 Dec 19, 2022
Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Please move this into scripts/config_check.

On Mac this doesn't work:

% ./refresh.sh
This WOLFSSL_REPO = /Users/davidgarske/GitHub/wolfssl
WOLFSSL_FILE_ROOT = /Users/davidgarske/GitHub/wolfssl/config_check
CMD File= /Users/davidgarske/GitHub/wolfssl/config_check/cmd.txt

tee: illegal option -- -
usage: tee [-ai] [file ...]

tee: illegal option -- -
usage: tee [-ai] [file ...]
tee: illegal option -- -
usage: tee [-ai] [file ...]
Error running command /Users/davidgarske/GitHub/wolfssl/config_check/cmd.txt

Mac uses -a for append -a Append the output to the files rather than overwriting them.. usage: tee [-ai] [file ...]

@dgarske dgarske assigned gojimmypi and unassigned dgarske Dec 23, 2022
@gojimmypi
Copy link
Contributor Author

Replaced tee --append with tee -a. I don't have a Mac so I was unable to test on a Mac. Verified on WSL and real Ubntun VM.

Renamed the refresh script to config_check.sh and moved into scripts directory.

Now that I re-read that, @dgarske perhaps you meant literally the scripts/config_check directory? Not a problem if you'd like to isolate it all in a new subdirectory of scripts.

Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Hey Jim,

Please move these files (named as they were) into a new directory scripts/config_check/.

The fix for tee -a works on my Mac.

Thanks

@gojimmypi
Copy link
Contributor Author

@dgarske - done; moved into scripts/config_check on this PR ConfigCheck branch.

Should I try to squash commits during or after the PR review process? There are some rather petty ones cluttering up the history. Let me know the best practice for this.

@gojimmypi gojimmypi marked this pull request as ready for review December 28, 2022 01:57

#### `WOLFSSL_CMD_FILE`

This the `.configure` command to edit. This is typically the location of the [cmd.txt](./cmd.txt) file
Copy link
Contributor

Choose a reason for hiding this comment

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

should be This is the not This the

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, I thought I fixed this, but there was another. good catch. thank you!

@gojimmypi
Copy link
Contributor Author

updated readme, resync with upstream and squashed commits, reopened for review.

@gojimmypi gojimmypi reopened this Dec 28, 2022
@dgarske dgarske marked this pull request as ready for review December 28, 2022 17:00
@kaleb-himes
Copy link
Contributor

Initial thoughts on testing this out and reading the output:

  1. Some of the comments seem hard-coded and if/when something is fixed will become obsolete

    • Example from cmd.txt -> --disable-md4 # !! this does not actually disable MD4 !! use NO_MD4
      Observation: Down the road items may get fixed or changed... are the comments in cmd.txt going to automatically update or will someone have to manually maintain this long term? (Q: What is the potential technical debt associated with this solution?)
  2. I've often needed to review things in the log which this solution seems to be getting at but my approach was to simply echo it to a file for review ./configure <options> > kaleb-config.log 2>&1 (NOTE: 2>&1 echo's stderr to stdout and all of stdout then gets directed to the file so everything is there when you need to review it)
    Q: Can you shed some light on what it is that this solution offers above and beyond a simple approach like the above?
    Q2: Is that additional offering worth the technical debt I suspect will result to maintain this and keep it up to date?

scripts/config_check/refresh.sh Show resolved Hide resolved
scripts/config_check/Disabled-Features.txt Show resolved Hide resolved
scripts/config_check/Enabled-Features.txt Outdated Show resolved Hide resolved
scripts/config_check/options.h Show resolved Hide resolved
scripts/config_check/output.txt Show resolved Hide resolved
scripts/include.am Show resolved Hide resolved
@dgarske dgarske removed their assignment Dec 28, 2022
Copy link
Contributor

@kaleb-himes kaleb-himes left a comment

Choose a reason for hiding this comment

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

Comment posted above.

@gojimmypi
Copy link
Contributor Author

gojimmypi commented Dec 28, 2022

@kaleb-himes I created this script mainly to have the ability to have a fully commented list of parameters to feed to the ./configure command. Sure, there are a variety of ways to compare the output. I personally like the separate lists of enabled and disabled features in separate files. In particular, I liked having the configuration history with my commits when it came time for memory footprint analysis.

Note that in my case, I was using it for an embedded project. So I was manually copying some generated settings from the ./configure to the project user_settings.h file. Keeping the ./configure parameters and result coordinated with my user_settings.h and code history was useful at the time.

Good point on the cmd.txt file; Your are correct that the sample as-is may not be appropriate, and in particular the comments. Do you have a suggestion for a good default configuration? Otherwise I can prune to something smaller with more obvious sample comments.

  • edit:

Q: What is the potential technical debt associated with this solution.

Minimal to none of there's no specific comments in the example. I've removed some of the specifics.

Q: Can you shed some light on what it is that this solution offers above and beyond a simple approach like the above?

convenience of a ./configure command with comments on each of the parameters and readily viewed files of currently enabled and disabled features.

Q: Is that additional offering worth the technical debt I suspect will result to maintain this and keep it up to date?

I don't think there's much technical debt here, particularly if the example cmd.txt is kept simple. This is just a script to run the ./configure command and output files for a given project that I believe are easier to read than the full output.

@gojimmypi
Copy link
Contributor Author

I appreciate the review and attention to detail. I've made some changes as suggested.

jpbland1
jpbland1 previously approved these changes Dec 30, 2022
@wolfSSL-Bot
Copy link

Can one of the admins verify this patch?

@gojimmypi
Copy link
Contributor Author

I've refreshed from upstream, squashed & re-added files; also fixed the line endings on the prior .gitignore that should have never been included (thanks @ejohnstown).

Let me know what needs to be done to get this one merged. Another issue came up regarding an embedded application needing particular settings in user_settings.h. I believe this script is a useful tool for seeing what gate macros change with the changes in command-line build options.

@gojimmypi
Copy link
Contributor Author

Jenkins retest this please

@gojimmypi
Copy link
Contributor Author

There are probably better tools available. I'm closing this lingering draft PR as I likely won't submit it in the near future.

@gojimmypi gojimmypi closed this Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants