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.

Sign up for GitHub

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

Disable large file support on old glibc #1910

Closed
wants to merge 3 commits into from
Closed

Disable large file support on old glibc #1910

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
207b611
Disable large file support on old glibc
sjmulder Jul 15, 2024
bb08543
Fix logic mistake
sjmulder Jul 16, 2024
3e801d3
Remove extraneous condition
sjmulder Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/nnn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,16 @@
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

#define _FILE_OFFSET_BITS 64 /* Support large files on 32-bit glibc */

#if defined(__linux__) || defined(MINGW) || defined(__MINGW32__) \
|| defined(__MINGW64__) || defined(__CYGWIN__)
#ifndef _GNU_SOURCE
#define _GNU_SOURCE
#endif
#include <features.h> /* __GLIBC__ etc */
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is a public header for us to be including.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't aware of this but indeed feature_test_macros(7) documents this. I can shuffle includes around a bit to avoid this if you're not against this PR a priori.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's upto @jarun to make the call. I personally don't think trying to work around a decade old glibc bug is worth it, especially since glibc maintainers now recommend defining it unconditionally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I understand the perspective but the one extra ifdef keeps nnn working on legacy platforms where system utilities like this remain useful. Note that RHEL 7 is still under extended lifecycle support. I'll keep it working in pkgsrc while I can but it would be nice if vanilla nnn kept working too.

Copy link
Owner

Choose a reason for hiding this comment

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

I am fine with this. We should try to keep nnn working on older systems.

Copy link
Collaborator

Choose a reason for hiding this comment

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

then unconditionally include one of the base headers like stddef.h to get the effects of features.h

This sounds okay.

That would assume it's okay to delay _FILE_OFFSET_BITS until after stddef.h

Unfortunately, I don't think this will work. From the feature_test_macros manpage:

"NOTE: In order to be effective, a feature test macro must be defined before including any header files"

Copy link
Collaborator Author

@sjmulder sjmulder Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

_FILE_OFFSET_BITS is indeed checked by features.h, so a bit of a chicken and egg problem here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't think of a way to do this correctly without a configure script. Unless you do I propose we close this PR and I think of something better (or leave it as is).

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a bit of pseudo configure script going on in the Makefile over here

nnn/Makefile

Lines 189 to 190 in 86d883e

# test if we are on Mac OS X and get X.Y.Z OS version with system binary /usr/bin/sw_vers
MACOS_VERSION := $(strip $(shell command -v sw_vers >/dev/null && [ "`sw_vers -productName`" = "Mac OS X" ] && sw_vers -productVersion))

If you can do something like this without too much baggage (we don't want to slow down the build step noticeably) then that seems acceptable to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here's a command that you can use to test glibc version in the makefile if you're still interested in pursuing this:

printf "#include <stdlib.h>\n#if __GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 23)\nYES\n#else\nNO\n#endif\n" | cc -E - | tail -n1

/* large file support on 32-bit glibc >= 2.23 where fts.h supports it */
#if __GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 23)
#define _FILE_OFFSET_BITS 64
#endif
#if defined(__linux__)
#include <sys/inotify.h>
#define LINUX_INOTIFY
Expand Down
Loading