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

Make Windows build more robust for the ONECORE variant #10586

Closed
wants to merge 2 commits into from
Closed

Make Windows build more robust for the ONECORE variant #10586

wants to merge 2 commits into from

Conversation

haohui
Copy link
Contributor

@haohui haohui commented Dec 7, 2019

I am trying to build OpenSSL on Windows and have discovered two issues that prevent a successful build of the ONECORE variant:

  1. The makefile fails to execute the binaries (e.g., CC / CPP) if there are spaces in the path.
  2. The makefile misses the checks of whether the files exist when calling copy.pl.

This PR addresses the two issues above and improve the robustness of the build system.

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Dec 7, 2019
@haohui haohui closed this Dec 7, 2019
@haohui haohui reopened this Dec 7, 2019
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Dec 7, 2019
levitte
levitte requested changes Dec 7, 2019
View reviewed changes
Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

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

This change should be made for all similar variables, not just a select few.

Configurations/windows-makefile.tmpl Outdated Show resolved Hide resolved
@haohui haohui force-pushed the robust-windows-build branch from 3b8c47c to 9edd875 Compare December 7, 2019 18:58
@haohui
Copy link
Contributor Author

haohui commented Dec 11, 2019

Just wondering, any plan to merge this PR?

@levitte levitte added approval: review pending This pull request needs review by a committer branch: master Merge to master branch labels Dec 11, 2019
@levitte
Copy link
Member

levitte commented Dec 11, 2019

We need another review, not just mine. @openssl/committers?

(I hadn't updated the labels, done now)

mspncp
mspncp approved these changes Dec 11, 2019
View reviewed changes
Copy link
Contributor

@mspncp mspncp left a comment

Choose a reason for hiding this comment

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

I checked the diffs, the changes look reasonable and correct. But I did not do any testing. Hope that suffices as a review @levitte.

@mspncp mspncp added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Dec 11, 2019
@levitte
Copy link
Member

levitte commented Dec 11, 2019

So FYI, @haohui, we apply a 24h grace period after the approval: done label has been set. In other words, unless some reviewer has a different opinion, this gets merged tomorrow, same time as now.

@paulidale paulidale added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Dec 15, 2019
openssl-machine pushed a commit that referenced this pull request Dec 19, 2019
@haohui @levitte
Make Windows build more robust
b575608 
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #10586)
@levitte
Copy link
Member

levitte commented Dec 19, 2019

Merged.

b575608 Make Windows build more robust

@levitte levitte closed this Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mspncp mspncp mspncp approved these changes

@levitte levitte levitte approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@haohui @levitte @mspncp @paulidale @openssl-machine