Fix build with musl and add Alpine buildnode to CI by kraj · Pull Request #174 · inotify-tools/inotify-tools

kraj · 2022-12-20T19:23:33Z

These functions are not needed on 64bit systems at all and also on 32bit systems with 64bit off_t

codecov · 2022-12-20T20:07:51Z

Codecov Report

Merging #174 (5e13d66) into master (f57005c) will increase coverage by 44.57%.
The diff coverage is 40.00%.

❗ Current head 5e13d66 differs from pull request most recent head 6e5e718. Consider uploading reports for the commit 6e5e718 to get more accurate results

@@             Coverage Diff             @@
##           master     #174       +/-   ##
===========================================
+ Coverage        0   44.57%   +44.57%     
===========================================
  Files           0        7        +7     
  Lines           0     2380     +2380     
===========================================
+ Hits            0     1061     +1061     
- Misses          0     1319     +1319

Impacted Files	Coverage Δ
src/common.c	`38.98% <0.00%> (ø)`
libinotifytools/src/inotifytools.c	`49.82% <100.00%> (ø)`
libinotifytools/src/redblack.c	`65.97% <0.00%> (ø)`
libinotifytools/src/test.c	`96.29% <0.00%> (ø)`
src/inotifywait.c	`53.26% <0.00%> (ø)`
libinotifytools/src/stats.c	`11.92% <0.00%> (ø)`
src/inotifywatch.c	`0.00% <0.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

kraj · 2022-12-20T22:12:40Z

The failure on ubuntu 20.04 seems to be unrelated. It runs clang-format on this file but the .clang-format policy in the repo wants to use 8-character tab and the files src/common.c and libinotifytools/src/inotifytools.c which are modified in this pull request does not seem to be formatted as per the policy even without the modification that this PR is doing to them.

sonarqubecloud · 2022-12-21T08:49:28Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

ericcurtin · 2022-12-21T10:37:11Z

.github/workflows/build.yml

+      GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+    steps:
+    - name: Setup latest Alpine Linux
+      uses: jirutka/setup-alpine@v1


I deliberately avoid these plugins, to avoid vendor lock-in, in case I have to migrate to GitLab or some other similar service in the future. We depend soley on free infrastructure, because we have zero funding.

I think you have the hard part done. Could we instead wrap in a podman or docker container directly and run this in one of the shell scripts?

The less .yml and plugins, the better!

An example is like here https://github.com/ericcurtin/twincam/blob/main/tests/run-all.sh

ericcurtin · 2022-12-21T10:38:22Z

build_and_test.sh


 if [ -n "$TRAVIS" ] || [ -n "$CI" ]; then
-  if [ "$os" != "freebsd" ]; then
+  if [ "${ID:-linux}" = "ubuntu" ] || [ "${ID:-linux}" = "debian" ]; then


This is where I'd like the alpine package installs to go, just to keep everything in the same place.

yeah if we do not use plugin.

ericcurtin · 2022-12-21T10:40:13Z

build_and_test.sh

 inotifytools_inc2="$inotifytools_inc/inotifytools"
 inc="-I$usr_inc -I$inotifytools_inc -I$inotifytools_inc2"

+if [ "${ID:-linux}" = "alpine" ]; then


Is it possible to fix the things pointed out as warnings instead of disabling the warning?

fixing is hard because for that it has to get definition of the structure which is the whole problem with sys/fanotify.h mess-up that various libc implementations have done. I tried to swap the elements as well but that has same problem. Because clang does not allow VLAIS it complains about it. Usually it should work ok.

Does this fix it? 94dfc73e7cf4a31da66b8843f0b9283ddd6b8381

index f1f89132d60e2..197df344307db 100644 --- a/include/uapi/linux/fanotify.h +++ b/include/uapi/linux/fanotify.h @@ -162,7 +162,7 @@ struct fanotify_event_info_fid { * Following is an opaque struct file_handle that can be passed as * an argument to open_by_handle_at(2). */ - unsigned char handle[0]; + unsigned char handle[]; };

maybe it doesn't, but I noticed they made this change in the kernel. I think the empty brackets is the modern way of doing it.

Yeah that fixes it alright, this is what should be done instead of ignoring that warning... I just tested it...

ericcurtin · 2022-12-21T10:41:21Z

src/common.h

-#define stat64 stat
-#define lstat64 lstat
-#ifdef ENABLE_FANOTIFY
+#if defined(__FreeBSD__) && defined(ENABLE_FANOTIFY)


ericcurtin · 2022-12-21T10:41:34Z

libinotifytools/src/inotifytools/inotify-nosys.h

@@ -13,11 +13,6 @@
 #include <sys/syscall.h>
 #include <unistd.h>



ericcurtin · 2022-12-21T10:41:42Z

libinotifytools/src/inotifytools/inotifytools.h

@@ -1,11 +1,6 @@
 #ifndef _inotifytools_H
 #define _inotifytools_H



ericcurtin · 2022-12-28T20:37:58Z

So if you look at the latest commit f57005c I put together an alpine container for you without the plugin, you just need to uncomment the line that says "alpine" and it's good to go.

This will define _FILE_OFFSET_BITS to be 64 if off_t is 64bit and we do not need to define lfs64 functions Signed-off-by: Khem Raj <[email protected]>

When build tree is outside source-tree its not able to find fanotify-dfid-name.h, add needed include paths on compiler commandline Signed-off-by: Khem Raj <[email protected]>

System detects to use sys/fnotify.h and then assumes glibc's definitions but musl has definitions of its own. perhaps portable thing would be to use linux/fnotify.h interface directly on linux irrespective of libc See the differences discussion here [1] [1] https://inbox.vuxu.org/musl/20191112220151.GC27331@x230/T/#ma8700992467200c8792e0fa8508eae656b81aeba Signed-off-by: Khem Raj <[email protected]>

lfs64 functions are not needed when off_t is 64-bit Additionally this fixes build with musl which does not export these functions without defining _LARGEFILE64_SOURCE Signed-off-by: Khem Raj <[email protected]>

Signed-off-by: Khem Raj <[email protected]>

Fixes -Wno-gnu-variable-sized-type-not-at-end for musl clang-format Signed-off-by: Khem Raj <[email protected]>

Signed-off-by: Khem Raj <[email protected]>

Signed-off-by: Eric Curtin <[email protected]>

ericcurtin · 2022-12-30T12:05:13Z

Merged, thanks for the contribution...

kraj force-pushed the largefile branch 6 times, most recently from fac3ed2 to 10a2cb3 Compare December 21, 2022 01:01

kraj mentioned this pull request Dec 21, 2022

error: ‘__kernel_fsid_t’ has no member named ‘val’ #155

Closed

kraj force-pushed the largefile branch 6 times, most recently from b35fcd2 to 2853b84 Compare December 21, 2022 01:45

kraj changed the title ~~Replace stat64/lstat64 with stat/lstat~~ Fix build with musl and add Alpine buildnode to CI Dec 21, 2022

kraj force-pushed the largefile branch 9 times, most recently from ebf87d4 to 5e13d66 Compare December 21, 2022 08:01

ericcurtin reviewed Dec 21, 2022

View reviewed changes

libinotifytools/src/inotifytools/inotify-nosys.h

@@ -13,11 +13,6 @@

#include <sys/syscall.h>

#include <unistd.h>

Copy link

Member

ericcurtin Dec 21, 2022

Choose a reason for hiding this comment

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

LGTM

ericcurtin reviewed Dec 21, 2022

View reviewed changes

libinotifytools/src/inotifytools/inotifytools.h

@@ -1,11 +1,6 @@

#ifndef _inotifytools_H

#define _inotifytools_H

Copy link

Member

ericcurtin Dec 21, 2022

Choose a reason for hiding this comment

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

LGTM

kraj added 7 commits December 28, 2022 23:21

configure: Add AC_SYS_LARGEFILE autoconf macro

df9a5d2

This will define _FILE_OFFSET_BITS to be 64 if off_t is 64bit and we do not need to define lfs64 functions Signed-off-by: Khem Raj <[email protected]>

Fix build for out-of-tree builds

9119374

When build tree is outside source-tree its not able to find fanotify-dfid-name.h, add needed include paths on compiler commandline Signed-off-by: Khem Raj <[email protected]>

replace stat64/lstat64 with stat/lstat

9605ad6

lfs64 functions are not needed when off_t is 64-bit Additionally this fixes build with musl which does not export these functions without defining _LARGEFILE64_SOURCE Signed-off-by: Khem Raj <[email protected]>

Apply changes from clang-format

c30a56f

Signed-off-by: Khem Raj <[email protected]>

Fix errors found by clang-format

3e01563

Fixes -Wno-gnu-variable-sized-type-not-at-end for musl clang-format Signed-off-by: Khem Raj <[email protected]>

ci: Enable musl in CI using alpine

b33f39b

Signed-off-by: Khem Raj <[email protected]>

kraj force-pushed the largefile branch from 5e13d66 to b33f39b Compare December 29, 2022 07:26

ericcurtin added 3 commits December 30, 2022 09:22

Tell clang-tidy to ignore warning about variable sized type

1a9b699

Signed-off-by: Eric Curtin <[email protected]>

Add gcompat for code coverage tool

338a094

Signed-off-by: Eric Curtin <[email protected]>

Skip sonarcloud on alpine

6e5e718

Signed-off-by: Eric Curtin <[email protected]>

ericcurtin merged commit 8367bdd into inotify-tools:master Dec 30, 2022

chenrui333 mentioned this pull request Aug 21, 2023

inotify-tools 4.23.8.0 Homebrew/homebrew-core#140114

Merged

		@@ -13,11 +13,6 @@
		#include <sys/syscall.h>
		#include <unistd.h>

		@@ -1,11 +1,6 @@
		#ifndef _inotifytools_H
		#define _inotifytools_H

Uh oh!

Comments

Conversation

kraj commented Dec 20, 2022

Uh oh!

codecov bot commented Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kraj commented Dec 20, 2022

Uh oh!

sonarqubecloud bot commented Dec 21, 2022

Uh oh!

ericcurtin Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericcurtin commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericcurtin commented Dec 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Dec 20, 2022 •

edited

Loading

ericcurtin Dec 21, 2022 •

edited

Loading

ericcurtin commented Dec 28, 2022 •

edited

Loading