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

Fix uninitialized value $s warning in windows static builds #6833

Closed
wants to merge 1 commit into from
Closed

Fix uninitialized value $s warning in windows static builds #6833

wants to merge 1 commit into from

Conversation

bernd-edlinger
Copy link
Member

Fixes: #6826

[extended tests]

@mattcaswell
Copy link
Member

Travis red cross appears unrelated.

@mattcaswell mattcaswell added the approval: done This pull request has the required number of approvals label Aug 2, 2018
@mattcaswell mattcaswell added this to the 1.1.1 milestone Aug 2, 2018
@levitte
Copy link
Member

levitte commented Aug 2, 2018 •
edited

-1

This isn't the right answer. There are a number of quotify1(shlib($whatever)), and those will return a quoted empty string where they shouldn't with this change. The right answer in this case should be to change quotify1(shlib to quotify_l(shlib in windows-makefile.tmpl

@bernd-edlinger
Copy link
Member Author

No problem, I can change the PR, or did you already have one?

@bernd-edlinger bernd-edlinger removed the approval: done This pull request has the required number of approvals label Aug 2, 2018
@levitte
Copy link
Member

levitte commented Aug 2, 2018

Feel free to go ahead

@bernd-edlinger
Copy link
Member Author

funny, quotify_l(@{shlib($_)}) did not make a warning, but does not print anything when it should.
I think I need to do my $l = shlib($_); quotify1($l) if defined($l);

@bernd-edlinger
Copy link
Member Author

okay, next try.

@bernd-edlinger
Copy link
Member Author

CI is good now.

levitte
levitte requested changes Aug 2, 2018
View reviewed changes
Configurations/windows-makefile.tmpl Outdated
@@ -93,8 +93,8 @@ GENERATED={- # common0.tmpl provides @generated
@generated) -}

INSTALL_LIBS={- join(" ", map { quotify1(lib($_)) } @{$unified_info{install}->{libraries}}) -}
INSTALL_SHLIBS={- join(" ", map { quotify1(shlib($_)) } @{$unified_info{install}->{libraries}}) -}
INSTALL_SHLIBPDBS={- join(" ", map { local $shlibext = ".pdb"; quotify1(shlib($_)) } @{$unified_info{install}->{libraries}}) -}
INSTALL_SHLIBS={- join(" ", map { my $l = shlib($_); quotify1($l) if defined($l) } @{$unified_info{install}->{libraries}}) -}
Copy link
Member

Choose a reason for hiding this comment

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

Why not just quotify_l(shlib($_))?

Copy link
Member Author

Choose a reason for hiding this comment

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

me no speak perlish...

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

@levitte
Copy link
Member

levitte commented Aug 2, 2018

funny, quotify_l(@{shlib($_)}) did not make a warning, but does not print anything when it should.

Well, @{shlib($_)} expects shlib to return an array reference rather than an array...

@levitte levitte added branch: master Merge to master branch 1.1.0 branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch labels Aug 2, 2018
levitte
levitte approved these changes Aug 2, 2018
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.

Looks good to me. Please remember to cherry-pick to 1.1.0 as well.

@bernd-edlinger bernd-edlinger added the approval: done This pull request has the required number of approvals label Aug 2, 2018
levitte pushed a commit that referenced this pull request Aug 2, 2018
@bernd-edlinger
Fix uninitialized value $s warning in windows static builds
d8a4f8f 
Fixes: #6826

[extended tests]

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #6833)
@bernd-edlinger
Copy link
Member Author

It did unfortunately not cherry-pick, I will send a new PR for 1.1.0 in a moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@levitte levitte levitte approved these changes

@mattcaswell mattcaswell mattcaswell approved these changes

Assignees
No one assigned
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch
Projects
None yet
Milestone
1.1.1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bernd-edlinger @mattcaswell @levitte