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.

Sign up for GitHub

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

Jump to bottom

include user_settings.h manually when wolfCrypt is not in use #285

Merged
merged 1 commit into from
Jul 27, 2023
Merged

include user_settings.h manually when wolfCrypt is not in use #285

merged 1 commit into from
Jul 27, 2023

Conversation

jpbland1
Copy link
Contributor

No description provided.

@jpbland1 jpbland1 marked this pull request as ready for review July 27, 2023 20:05
@jpbland1 jpbland1 requested a review from dgarske July 27, 2023 20:05
dgarske
dgarske approved these changes Jul 27, 2023
View reviewed changes
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.

Thanks for putting up my patch suggestion. @lealem47 please review as well

@dgarske dgarske assigned lealem47 and unassigned dgarske Jul 27, 2023
@lealem47
Copy link
Contributor

lealem47 commented Jul 27, 2023 •
edited
Loading

Why not move this outside of the WOLFTPM2_NO_WOLFCRYPT guard? That way users can have a separate user_settings.h for configuring wolfTPM instead of modifying their wolfSSL user settings. Same way we do wolfSSH, wolfMQTT, ...

lealem47
lealem47 approved these changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@lealem47 lealem47 left a comment

Choose a reason for hiding this comment

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

Just realized that my suggestion would break current builds that use WOLFTPM_USER_SETTINGS and don't have a user_settings.h present. It would a quick fix with $ touch user_setting.h but I get that we'd want to avoid that

@lealem47 lealem47 assigned dgarske and unassigned lealem47 Jul 27, 2023
@dgarske
Copy link
Contributor

dgarske commented Jul 27, 2023

Just realized that my suggestion would break current builds that use WOLFTPM_USER_SETTINGS and don't have a user_settings.h present. It would a quick fix with $ touch user_setting.h but I get that we'd want to avoid that

This use case would only happen if trying to build wolfTPM without wolfCrypt and not using configure so it would require some type of build settings to be provided. Do you think this could be an issue for anyone in the "wild"?

@lealem47
Copy link
Contributor

Just realized that my suggestion would break current builds that use WOLFTPM_USER_SETTINGS and don't have a user_settings.h present. It would a quick fix with $ touch user_setting.h but I get that we'd want to avoid that

This use case would only happen if trying to build wolfTPM without wolfCrypt and not using configure so it would require some type of build settings to be provided. Do you think this could be an issue for anyone in the "wild"?

The current patch is fine, I'd guess a very limited number of users with this setup. I was talking about this suggestion #285 (comment) to always include user_settings.h even with wolfCrypt when WOLFTPM_USER_SETTINGS is defined

@dgarske dgarske merged commit 7c9391e into wolfSSL:master Jul 27, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgarske dgarske dgarske approved these changes

@lealem47 lealem47 lealem47 approved these changes

Assignees

@dgarske dgarske

@jpbland1 jpbland1

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jpbland1 @lealem47 @dgarske