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

Makefile.m32: fix to not require OpenSSL with options -libssh2 and -rtmp #7895

Closed
wants to merge 1 commit into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 23, 2021

Previously, -libssh2/-rtmp options assumed that OpenSSL is also enabled (and then failed with an error when not finding expected OpenSSL headers), but this isn't necessarly true, e.g. when building both libssh2 and curl against Schannel. This patch makes sure to only enable the OpenSSL backend with -libssh2/-rtmp, when there was no SSL option explicitly selected.

  • Re-implement the logic as a single block of script.
  • Also fix an indentation while there.

Assisted-by: Jay Satiro

Closes #


This has been tested locally and as part of curl-for-win.

@vszakats vszakats added build Windows Windows-specific labels Oct 23, 2021
@vszakats
Copy link
Member Author

CI failures look random and unrelated.

Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

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

Previously the -libssh2 option assumed that OpenSSL is also enabled (and failed with an error when not finding expected OpenSSL headers), but this isn't necessarly true when building both libssh2 and curl against Schannel.

Is this also true for RTMP which sets SSL = 1?

ifneq ($(findstring -winssl,$(CFG))$(findstring -ssl,$(CFG)),-winssl)

What happens if the user asked for both? then won't this evaluate to -winssl-ssl != -winssl. trivial I guess since all it does is set SSL =1. Have you considered removing SSL = 1 when a build option needs it and instead checking that after all the build options are set?

# SSH2 and RTMP require an SSL library; assume OpenSSL if none specified
ifneq ($(or $(SSH2),$(RTMP)),)
  ifeq ($(or $(SSL),$(WINSSL)),)
    SSL = 1
  endif
endif

(I'm not great with gnu make and I based the conditional logic on what I read here), edit: modified 2021-10-24

On the other hand maybe we could require the user to specify an SSL lib?

@vszakats
Copy link
Member Author

@jay: I wanted to keep this as simple as possible because GNU Make is fragile and also wanted to maintain compatibility. I'm not doing tests with RTMP and I'm also not very familiar with it, so while I noticed a similar possible issue there, I did not address it in this patch. Ideally we should not enable any implied (= guessed) SSL backend of dependencies, as those can be built against any number of backends regardless of what SSL backend is used for curl itself 1, but removing these might break compatibility (ie. scripts enabling -libssh2 or -rtmp without -ssl).

If the user asked for both -ssl and -winssl, the matching internal flags (SSL and WINSSL) will be both set by the init logic for these two specific flags (and only by these). It's redundant to set any of them also inside the -libssh2 conditional in that case. This is the scenario used by curl-for-win, also tested before opening this PR.

I avoided $(or) because it breaks compatibility with version GNU Make 3.79 (and 3.80). Which is similar in ubiquity as C89. else if[n]eq also came with 3.81, and it isn't used in curl GNU Make files.

This would be this convencience-logic unified as you suggest, but with 3.79 compatibility retained (I haven't tested it):

# SSH2 and RTMP require an SSL library; assume OpenSSL if none specified
ifneq ($(SSH2)$(RTMP),)
  ifeq ($(SSL)$(WINSSL),)
    SSL = 1
  endif
endif

I'd feel more confortable to commit this as-is, and the above could be committed afterwards as an improvement to include RTMP and to keep this logic in a single block.

Footnotes

  1. For example a few years ago MSYS2 built curl against OpenSSL and RTMP, and RTMP was built against GnuTLS.

@vszakats
Copy link
Member Author

vszakats commented Oct 25, 2021

@jay: Update: I'm re-testing the two scenarios with your proposal (the 3.79-compatible flavour) and if these are okay, we can commit it right away.

@vszakats vszakats changed the title Makefile.m32: fix libssh2 + Schannel build combination Makefile.m32: fix to not require OpenSSL with options -libssh2 and -rtmp Oct 25, 2021
@vszakats
Copy link
Member Author

Both the local and CI tests were fine (the build had an unrelated failure, hence red.)

Previously, -libssh2/-rtmp options assumed that OpenSSL is also enabled
(and then failed with an error when not finding expected OpenSSL headers),
but this isn't necessarly true, e.g. when building both libssh2 and curl
against Schannel. This patch makes sure to only enable the OpenSSL backend
with -libssh2/-rtmp, when there was no SSL option explicitly selected.

- Re-implement the logic as a single block of script.
- Also fix an indentation while there.

Assisted-by: Jay Satiro

Closes: #
jay
jay approved these changes Oct 25, 2021
Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

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

I avoided $(or) because it breaks compatibility with version GNU Make 3.79 (and 3.80). Which is similar in ubiquity as C89. else if[n]eq also came with 3.81, and it isn't used in curl GNU Make files.

Ok. FWIW I have an old version of mingw32-make from 2012 and the version is GNU Make 3.82.90. (Edit: found another one from 2010 GNU Make 3.82)

Minor nit: The commit message uses Closes: and I don't know if that will work as a keyword when there's a colon.

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.

None yet

3 participants