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

[testing] Update linter to more recent component versions #29

Merged
merged 8 commits into from
Aug 3, 2023

Conversation

leifmadsen
Copy link
Member

@leifmadsen leifmadsen commented Aug 1, 2023

Closes #28

@leifmadsen leifmadsen self-assigned this Aug 1, 2023
@leifmadsen leifmadsen marked this pull request as ready for review August 3, 2023 02:59
@leifmadsen
Copy link
Member Author

I believe these changes are correct after additional investigation. I spoke with Release Delivery and they required this flag as well for some downstream builds. My understanding is the gaps in the notes that annobin/annocheck uses are failing on something in proton, but it's not clear (to me) why that is. It seems to be limited to a single function at https://github.com/infrawatch/sg-bridge/blob/master/amqp_rcv_th.c#L37-L46

Hints came from the following documents:

The latter suggests this should really only be used against self-written assembly, but I'm comfortable enough with the risk here. Additionally we now added the -flto to result in a previous failure about not being fully LTO compatible (ref: https://gcc.gnu.org/wiki/LinkTimeOptimization).

Most of these concepts are beyond my knowledge realm, and am throwing a wide audience to determine if there might be some other action item here, but in the end, all I wanted to do was move our linting test from fedora:32 base image to fedora:latest. Doing that caused an updated annocheck which added new checks we previously weren't detecting.

@@ -2,8 +2,8 @@

set -ex

dnf install make gcc qpid-proton-c-devel annobin-annocheck rpm-build -y
dnf install make gcc qpid-proton-c-devel annobin-annocheck gcc-plugin-annobin rpm-build -y
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this in previous configuration where I was attempting to test with -fplugin=annobin but no changes were had. I don't think this is strictly necessary, but it seems lack a valid tool to have available.

@leifmadsen leifmadsen requested a review from aneeshkp August 3, 2023 03:09
After further investigation it was found that the Makefile was missing
calls to CFLAGS which was causing issues with notes generation. That has
now been fixed.

Also the CFLAGS and LDFLAGS have been updated to match the latest best
values provided to us by release-delivery.

Additionally the CFLAGS and LDFLAGS configuration have been moved out of
the external build_checks.sh script into the Makefile directly so that
the build_checks script can just run make directly, as well as setting
proper defaults for how we expect sg-bridge to be built.

Signed-off-by: Leif Madsen <lmadsen@redhat.com>
@leifmadsen
Copy link
Member Author

OK I'm wrapping this up now after getting a review and help from Nick Clifton who understands annocheck really well.

The final set of changes was primarily around the Makefile, where it turns out we were missing calls to CFLAGS that were required after implementing LTO.

From Nick Clifton

This is because you are using LTO - so all of the real compilation is actually done during the final link, and this compilation needs to be told which gcc compile options to use. such as -fstack-clash-protection and -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1

@leifmadsen leifmadsen merged commit ede398f into master Aug 3, 2023
@leifmadsen leifmadsen deleted the update/gha-linter branch August 3, 2023 19:26
leifmadsen added a commit that referenced this pull request Aug 15, 2023
When making fixes in #29 related to lint checks, a local build wasn't
done against the Dockerfile, which has a different base image from that
used by build_checks for lint verification. The result of that was that
building sg-bridge with the Dockerfile failed on gcc compiling due to
hardening instructions not being available.

This commit does the following:

* adds the RPM dependency to the build layer of Dockerfile to install
  hardening instructions for gcc
* changes the default -march CFLAG to be x86-64 instead of x86-64-v2
  which fails in ubi8
* updates the GHA to add a build check against the Dockerfile and not
  just run build_checks.sh for future validation

Found by csibbitt and noted in chat.

Resolves #30

Signed-off-by: Leif Madsen <lmadsen@redhat.com>
csibbitt pushed a commit that referenced this pull request Aug 16, 2023
When making fixes in #29 related to lint checks, a local build wasn't
done against the Dockerfile, which has a different base image from that
used by build_checks for lint verification. The result of that was that
building sg-bridge with the Dockerfile failed on gcc compiling due to
hardening instructions not being available.

This commit does the following:

* adds the RPM dependency to the build layer of Dockerfile to install
  hardening instructions for gcc
* changes the default -march CFLAG to be x86-64 instead of x86-64-v2
  which fails in ubi8
* updates the GHA to add a build check against the Dockerfile and not
  just run build_checks.sh for future validation

Found by csibbitt and noted in chat.

Resolves #30

Signed-off-by: Leif Madsen <lmadsen@redhat.com>
@leifmadsen leifmadsen mentioned this pull request Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Bump Fedora version in linter test
1 participant