Skip to content

Use $config{build_file} instead of $target{build_file}#20173

Closed
levitte wants to merge 3 commits intoopenssl:masterfrom
levitte:fix-buildfile-settings
Closed

Use $config{build_file} instead of $target{build_file}#20173
levitte wants to merge 3 commits intoopenssl:masterfrom
levitte:fix-buildfile-settings

Conversation

@levitte
Copy link
Member

@levitte levitte commented Jan 30, 2023

If the user specifies an alternative build file than the default, this
alternative is recorded in $config{build_file}, not $target{build_file}.
Therefore, the former should be used, leaving the latter as a mere default.

This is a bug. While fixing it, document it better too.

@levitte
Use $config{build_file} instead of $target{build_file}
a1b6b1f 
If the user specifies an alternative build file than the default, this
alternative is recorded in $config{build_file}, not $target{build_file}.
Therefore, the former should be used, leaving the latter as a mere default.

This is a bug.  While fixing it, document it better too.
@levitte levitte added approval: otc review pending approval: review pending This pull request needs review by a committer branch: 3.0 Applies to openssl-3.0 branch branch: 3.1 Applies to openssl-3.1 (EOL) branch: master Applies to master branch labels Jan 30, 2023
@levitte levitte self-assigned this Jan 30, 2023
@levitte levitte requested review from a team January 30, 2023 12:59
@t8m t8m added triaged: bug The issue/pr is/fixes a bug tests: exempted The PR is exempt from requirements for testing labels Jan 30, 2023
t8m
t8m previously approved these changes Jan 30, 2023
View reviewed changes
@levitte
Copy link
Member Author

levitte commented Jan 30, 2023

This should cherry-pick nicely to 3.1 and 3.0. 1.1.1 requires a
separate PR.

@levitte levitte mentioned this pull request Jan 30, 2023
Closed
2 tasks
mattcaswell
mattcaswell requested changes Jan 30, 2023
View reviewed changes
@levitte levitte mentioned this pull request Jan 30, 2023
Closed
@levitte
Copy link
Member Author

levitte commented Jan 30, 2023

#20174 is the corresponding change for 1.1.1. It's more or less the same, minus documentation (which is different, and interestingly enough more complete for this stuff)

@ajkhoury
Copy link
Contributor

ajkhoury commented Jan 30, 2023

There is still a slight issue with this fix, the outputted files are still named as defined by $target{build_file}. You still need to change one reference in configdata.pm.in: https://github.com/openssl/openssl/blob/master/configdata.pm.in#L94

mattcaswell
mattcaswell previously approved these changes Jan 30, 2023
View reviewed changes
@mattcaswell
Copy link
Member

mattcaswell commented Jan 30, 2023

I assume @t8m's approval still holds since it was a trivial fix up.

@mattcaswell mattcaswell 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 Jan 30, 2023
ajkhoury
ajkhoury suggested changes Jan 30, 2023
View reviewed changes
Copy link
Contributor

@ajkhoury ajkhoury left a comment

Choose a reason for hiding this comment

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

Need to update reference here as well: https://github.com/openssl/openssl/blob/master/configdata.pm.in#L94

@levitte
Copy link
Member Author

levitte commented Jan 30, 2023

Need to update reference here as well: https://github.com/openssl/openssl/blob/master/configdata.pm.in#L94

Oh! Right you are

@levitte
Copy link
Member Author

levitte commented Jan 30, 2023

Unfortunately, that means re-approvals are needed

@levitte levitte requested review from mattcaswell and t8m January 30, 2023 17:44
@levitte levitte requested a review from ajkhoury January 30, 2023 17:44
@levitte levitte dismissed stale reviews from mattcaswell and t8m January 30, 2023 17:44

Changes

@levitte levitte added approval: review pending This pull request needs review by a committer approval: otc review pending and removed approval: done This pull request has the required number of approvals labels Jan 30, 2023
@t8m t8m 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 Jan 30, 2023
@openssl-machine openssl-machine 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 Jan 31, 2023
@openssl-machine
Copy link
Collaborator

openssl-machine commented Jan 31, 2023

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented Feb 1, 2023

Merged to master, 3.1, and 3.0 branches. Thank you.

@t8m t8m closed this Feb 1, 2023
openssl-machine pushed a commit that referenced this pull request Feb 1, 2023
@levitte @t8m
Use $config{build_file} instead of $target{build_file}
b0bd7a2 
If the user specifies an alternative build file than the default, this
alternative is recorded in $config{build_file}, not $target{build_file}.
Therefore, the former should be used, leaving the latter as a mere default.

This is a bug.  While fixing it, document it better too.

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #20173)

(cherry picked from commit aa2d7e0)
openssl-machine pushed a commit that referenced this pull request Feb 1, 2023
@levitte @t8m
Use $config{build_file} instead of $target{build_file}
aa2d7e0 
If the user specifies an alternative build file than the default, this
alternative is recorded in $config{build_file}, not $target{build_file}.
Therefore, the former should be used, leaving the latter as a mere default.

This is a bug.  While fixing it, document it better too.

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #20173)
openssl-machine pushed a commit that referenced this pull request Feb 1, 2023
@levitte @t8m
Use $config{build_file} instead of $target{build_file}
28b78f3 
If the user specifies an alternative build file than the default, this
alternative is recorded in $config{build_file}, not $target{build_file}.
Therefore, the former should be used, leaving the latter as a mere default.

This is a bug.  While fixing it, document it better too.

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #20173)

(cherry picked from commit aa2d7e0)
@levitte levitte deleted the fix-buildfile-settings branch February 1, 2023 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t8m t8m t8m approved these changes

@mattcaswell mattcaswell mattcaswell approved these changes

+1 more reviewer

@ajkhoury ajkhoury ajkhoury approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@levitte levitte

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch branch: 3.0 Applies to openssl-3.0 branch branch: 3.1 Applies to openssl-3.1 (EOL) tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@levitte @ajkhoury @mattcaswell @openssl-machine @t8m