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

Don't include sys/select.h on HP-UX as it doesn't exist #18395

Closed
wants to merge 1 commit into from

Conversation

tomhughes
Copy link
Contributor

There is no sys/select.h on HP-UX and the man page lists sys/time.h as the correct header for select.

@t8m t8m added branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch approval: review pending This pull request needs review by a committer labels May 24, 2022
@mattcaswell
Copy link
Member

I'm wondering why we haven't hit this before on this platform?

@rsbeckerca
Copy link
Contributor

I'm wondering why we haven't hit this before on this platform?

We have the same situation on HPE NonStop. sys/time.h is where select is declared also. I recall encountering it during our report.

@tomhughes
Copy link
Contributor Author

That code is new in 3.x I think - the only place in 1.1 that includes it is include/internal/sockets.h which does this:

#  ifdef OPENSSL_SYS_AIX
#   include <sys/select.h>
#  endif

which is only used on AIX systems.

@rsbeckerca
Copy link
Contributor

Is this on master or openssl-3.0? We just built 3.0 last night without errors.

@tomhughes
Copy link
Contributor Author

This came from me building the 3.0.3 release but looking at it I can see our Itanium machine (running B.11.23) does have it - it's only the PA-RISC machine (running B.11.11) which doesn't.

Both build without including it though.

@rsbeckerca
Copy link
Contributor

This came from me building the 3.0.3 release but looking at it I can see our Itanium machine (running B.11.23) does have it - it's only the PA-RISC machine (running B.11.11) which doesn't.

Both build without including it though.

I'm rerunning the builds on our ia64 also. The x86 build 3.0 was fine last night.

@mattcaswell
Copy link
Member

Is this on master or openssl-3.0? We just built 3.0 last night without errors.

There is specific code for NonStop to avoid this:

# if defined __TANDEM
#  include <unistd.h>
#  include <sys/time.h> /* select */
#  if defined(OPENSSL_TANDEM_FLOSS)
#   include <floss.h(floss_select)>
#  endif
# elif defined _WIN32
#  include <winsock.h> /* for type fd_set */
# else
#  include <unistd.h>
#  if defined __VMS
#   include <sys/socket.h>
#  else
#   include <sys/select.h>
#  endif
# endif

@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 May 24, 2022
@rsbeckerca
Copy link
Contributor

Is this on master or openssl-3.0? We just built 3.0 last night without errors.

There is specific code for NonStop to avoid this:

# if defined __TANDEM
#  include <unistd.h>
#  include <sys/time.h> /* select */
#  if defined(OPENSSL_TANDEM_FLOSS)
#   include <floss.h(floss_select)>
#  endif
# elif defined _WIN32
#  include <winsock.h> /* for type fd_set */
# else
#  include <unistd.h>
#  if defined __VMS
#   include <sys/socket.h>
#  else
#   include <sys/select.h>
#  endif
# endif

Yup. That looks like my handiwork ;)

@rsbeckerca
Copy link
Contributor

On the other hand, @mattcaswell, we are getting

make: *** No rule to make target 'include/openssl/configuration.h', needed by 'build_generated'.  Stop.

as of this morning.

@rsbeckerca
Copy link
Contributor

On the other hand, @mattcaswell, we are getting

make: *** No rule to make target 'include/openssl/configuration.h', needed by 'build_generated'.  Stop.

as of this morning.

That one results from make clean removing configuration.h

@mattcaswell
Copy link
Member

That one results from make clean removing configuration.h

Ah. Can you raise a separate issue for that please?

@rsbeckerca
Copy link
Contributor

That one results from make clean removing configuration.h

Ah. Can you raise a separate issue for that please?

Will do.

@rsbeckerca
Copy link
Contributor

Opened as #18396

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@t8m t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge cla: trivial One of the commits is marked as 'CLA: trivial' and removed approval: done This pull request has the required number of approvals labels May 27, 2022
openssl-machine pushed a commit that referenced this pull request May 27, 2022
CLA: trivial

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #18395)
@t8m
Copy link
Member

t8m commented May 27, 2022

I've merged this to master and 3.0 branches. I did not notice this has CLA:trivial before merging this but I am OK with it. @mattcaswell Can you please confirm, you're OK with CLA: trivial for this?

openssl-machine pushed a commit that referenced this pull request May 27, 2022
CLA: trivial

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #18395)

(cherry picked from commit 737e849)
@rsbeckerca
Copy link
Contributor

This builds correctly on both master and openssl-3.0 on NonStop x86 and ia64

@t8m t8m closed this Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch cla: trivial One of the commits is marked as 'CLA: trivial' triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants