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

Fix test code to can be built on alpine #4969

Merged

Conversation

jclab-joseph
Copy link
Contributor

Signed-off-by: joseph joseph@jc-lab.net

Description

When build via cmake on alpine linux, cause the following error: This PR fix the error.

[ 99%] Building C object tests/CMakeFiles/test_suite_net.dir/test_suite_net.c.o
In file included from .../tests/suites/test_suite_net.function:12:
/usr/include/sys/fcntl.h:1:2: error: #warning redirecting incorrect #include <sys/fcntl.h> to <fcntl.h> [-Werror=cpp]
    1 | #warning redirecting incorrect #include <sys/fcntl.h> to <fcntl.h>
      |  ^~~~~~~
cc1: all warnings being treated as errors

Status

READY

Requires Backporting

NO

Migrations

If there is any API change, what's the incentive and logic for it.

NO

Steps to test or reproduce

Before

https://github.com/jclab-joseph/mbedtls/actions/runs/1264843158

The build succeeds in ubuntu but fails in alpine.

After

https://github.com/jclab-joseph/mbedtls/actions/runs/1264845085

Both ubuntu and alpine builds succeed.

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

The obvious concern with this change is portability. library/net_sockets.c uses fcntl.h. So this reconciles the tests with the library. Besides sys/fcntl.h is long obsolete (for example FreeBSD switch to fcntl.h between 1.1 and 2.0, i.e. in the early 1990s). Hence, approved, thanks for fixing this.

Since this is a bug fix, even if it's only in the tests, I think this should have a changelog entry. Please create a file in ChangeLog.d (see the readme)) containing something like “Don't use the obsolete header path sys/fcntl.h in unit tests.”

Please make a similar patch to the long-time support branches: development_2.x and mbedtls-2.16.

@gilles-peskine-arm gilles-peskine-arm added bug component-platform Portability layer and build scripts needs-backports Backports are missing or are pending review and approval. needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Sep 23, 2021
Copy link
Contributor Author

@jclab-joseph jclab-joseph left a comment

Choose a reason for hiding this comment

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

Added Change.
In mbedtls-2.16 this test does not exist.

Signed-off-by: joseph <joseph@jc-lab.net>
@daverodgman daverodgman self-requested a review September 24, 2021 09:50
@daverodgman daverodgman removed the needs-reviewer This PR needs someone to pick it up for review label Sep 24, 2021
Copy link
Contributor

@daverodgman daverodgman left a comment

Choose a reason for hiding this comment

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

LGTM

@daverodgman daverodgman added size-s Estimated task size: small (~2d) and removed needs-backports Backports are missing or are pending review and approval. needs: changelog labels Sep 24, 2021
@gilles-peskine-arm gilles-peskine-arm added needs-backports Backports are missing or are pending review and approval. and removed needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. labels Sep 24, 2021
@gilles-peskine-arm gilles-peskine-arm merged commit 9e4c020 into Mbed-TLS:development Sep 24, 2021
gilles-peskine-arm added a commit that referenced this pull request Sep 24, 2021
Backport 2.x: Fix test code to can be built on alpine #4969
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-platform Portability layer and build scripts size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants