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

Document the minimum version for dependencies #307

Open
5 of 10 tasks
N-R-K opened this issue May 22, 2023 · 7 comments
Open
5 of 10 tasks

Document the minimum version for dependencies #307

N-R-K opened this issue May 22, 2023 · 7 comments
Labels
docs Documentation related

Comments

@N-R-K
Copy link
Collaborator

N-R-K commented May 22, 2023

Scrot currently has a bunch of library dependencies. But there's no indication of what's the minimum version we require. I highly doubt we'll be able to build and work fine on any versions of those libraries.

As such, we should try to figure out what our the minimum version requirement really is (or use a version that's reasonably high but not too bleeding-edge) and document them accordingly.


Lowest known/likely working versions of scrot's dependencies:

  • Imlib2: v1.4.7
  • Xlib: unknown
  • libXcomposite: v0.2.0
  • libXext: unknown
  • libXfixes: v5.0.1
  • libXinerama: v1.1.3
  • libbsd: unknown

build dependencies:

  • autoconf: unknown
  • autoconf-archive: unknown
  • pkg-config: there's multiple implementation, so minimum version is meaningless.
@N-R-K N-R-K added the docs Documentation related label May 22, 2023
@N-R-K
Copy link
Collaborator Author

N-R-K commented May 22, 2023

Aside from version, we might also have to document the specific features of the library as well. For example imlib2 allows for disabling text, filters and even X - but scrot requires them.

https://github.com/gentoo/gentoo/blob/7b2d22ddfcc3f65087d07a3f55408e652cd21dd0/media-gfx/scrot/scrot-1.9.ebuild#L24

Gentoo deals with this via USE flags for example.

@N-R-K

This comment was marked as off-topic.

@N-R-K
Copy link
Collaborator Author

N-R-K commented May 24, 2023

@guijan Sorry for dodging the question earlier (it's a complex question and I didn't have a short answer) - I'll try to answer this here (since it's somewhat related to this issue).

We should also figure out what systems we're willing to support.
I'm happy to work on supporting any libre system that is currently maintained. Even Termux inside DivestOS running Linux 3.0, that's still actively maintained, even though Linux 3.0 is abandoned by kernel.org.

In a vacuum supporting a large number of platforms sounds good, but it also has (sometimes non-trivial amount of) cost associated with it. For e.g I've seen some codebases with as many ifdefs as lines of code (and these are not fun to work with).

So in short, instead of trying to support specific implementation/platforms, we should support a specific baseline standard instead. More or less what this means is:

  • Compilers that support C99 (though we should avoid VLAs)
  • Platforms that support POSIX 2008 + XSI extensions (we document this in CONTRIBUTING, should do this in README as well) as well as being able to properly support the minimum version our library/interface dependencies.

Special exceptions can always be made for platform support (e.g if multiple platform lack a standard function).

I'll also work on any actively maintained proprietary system that we can have CI access to.

You perhaps already know this, but I'll point it out just in case: just because something built successfully doesn't mean it'll work as intended. For example, it's not uncommon to have stub functions that always return ENOSYS, especially in libcs/platforms in early development. (Of course, it can legitimately return ENOSYS as well if the kernel is too old).

So just because we can build scort in a CI doesn't mean we can claim support for that platform. (Note that this is not discouraging adding more niche platforms to our CI - that'd be nice. But we should be careful about claiming actual support since that means more than just "it compiles".)

@guijan
Copy link
Contributor

guijan commented May 24, 2023

It's really not bad at all if you do it like this: https://www.openbsd.org/papers/portability.pdf

Portability code should be hidden away in some compatibility layer. The entire program, save for a compat/ directory, has little to no #ifdef (can't avoid having a CONTINUOUS_CLOCK because of Linux, for instance). Later, you realize a platform lacks a function the program uses, and you peek into compat/ and there's a fallback there. If a platform is missing a standard (e.g. clock_nanosleep()) or vendor function (e.g. err()), you provide your own implementation. If a particular task is done with a different function in each platform, you think up an interface that encompasses all platforms, and implement it for each OS. In all cases, the program itself and the bits of glue that allow it to run on many platforms are well separated. The program is readable and intelligible.

The problem with sticking to a standard is that the program will run into platforms that have incomplete or broken implementations and avoid the interface on all platforms, or the program will refrain from using a good vendor function and opt for the bad standard function. Instead of elevating the bad platforms to the program's good practices, the program lowers itself to the worst of every platform, on all platforms.

I'd really love to save some filesize on NetBSD by using its ecalloc() instead of our own, use the proper sleep function on Linux (clock_nanosleep()) instead of some kludge that it doesn't need, fix strtok() skipping over a sequence of multiple separators by using strsep(), add OpenBSD's pledge(), use memrchr() if we feel like it, etc.

@N-R-K
Copy link
Collaborator Author

N-R-K commented May 24, 2023

Just a quick reply to one of the points above (I'll read the pdf and reply to the rest later):

I'd really love to save some filesize on NetBSD by using its ecalloc() instead of our own

Removing the e* definitions in utils.c and replacing them with #defines to the standard ones:

$ git diff src/util.h
diff --git a/src/util.h b/src/util.h
index 6cc7c78..eec771d 100644
--- a/src/util.h
+++ b/src/util.h
@@ -60,9 +60,9 @@ typedef struct {
     size_t off, cap;
 } Stream;
 
-char *estrdup(const char *);
-void *ecalloc(size_t, size_t);
-void *erealloc(void *, size_t);
+#define estrdup strdup
+#define ecalloc calloc
+#define erealloc realloc
 
 void streamReserve(Stream *, size_t);
 void streamChar(Stream *, char);
$ gcc -o scrot-extern src/*.c $(pkg-config --cflags --libs ./scrot-deps.pc) -Oz -flto -march=native
$ git restore .
$ gcc -o scrot-intern src/*.c $(pkg-config --cflags --libs ./scrot-deps.pc) -Oz -flto -march=native
$ wc -c scrot-*
  49912 scrot-extern
  49944 scrot-intern

Even on -O2 and -O3 it doesn't reach triple digit difference in bytes.

IMO, this is not even worth thinking about - let alone an ifdef (even if hidden away in some compat/ dir).

@guijan
Copy link
Contributor

guijan commented May 24, 2023

Edit: I was thinking about the wrong thing, I rewrote this comment.

Makes sense, the e* functions just check the return value and call err(), and they have the same prototype as the functions they error check, that is only a few bytes of extra machine code for the checking.

N-R-K added a commit that referenced this issue May 29, 2023
text is required due to --note
filters is required due to --script

ref: #307
N-R-K added a commit to N-R-K/scrot that referenced this issue Jun 19, 2023
this avoids misleading people due to the issues described in resurrecting-open-source-projects#315.

and packagers can already check the output of the configure script or
config.log to see if libbsd was checked for or not.

additionally, document which bsd extension we require in the README so
that maintainers can make more informed decision (e.g using more
lightweight `sys/queue.h` in musl instead of heavyweight libbsd).

ref: resurrecting-open-source-projects#307
Fixes: resurrecting-open-source-projects#315
N-R-K added a commit to N-R-K/scrot that referenced this issue Jun 19, 2023
this avoids misleading people due to the issues described in resurrecting-open-source-projects#315.

and packagers can already check the output of the configure script or
config.log to see if libbsd was checked for or not.

additionally, document which bsd extension we require in the README so
that maintainers can make more informed decision (e.g using more
lightweight `sys/queue.h` in musl instead of heavyweight libbsd).

ref: resurrecting-open-source-projects#307
Fixes: resurrecting-open-source-projects#315
N-R-K added a commit that referenced this issue Jun 20, 2023
this avoids misleading people due to the issues described in #315.

and packagers can already check the output of the configure script or
config.log to see if libbsd was checked for or not.

additionally, document which bsd extension we require in the README so
that maintainers can make more informed decision (e.g using more
lightweight `sys/queue.h` in musl instead of heavyweight libbsd).

ref: #307
Fixes: #315
@N-R-K
Copy link
Collaborator Author

N-R-K commented Jul 2, 2023

I was testing out lowest known imlib2 version for nsxiv and decided to try out scrot as well.

It seems that the lowest confirmed imlib2 version scrot can build with is v1.4.7 which was released in 2015-04-04. (Imlib2 v1.4.6 wouldn't build for me, though I didn't really troubleshoot the build error at all).


Going by the git history of XComposite, v0.2.0 from 2005 seem to have the functions that we use. So let's just go with that. (I didn't do any runtime testing, though).


Whether XFixes uses semantic versioning is not clear. But in any case, there was a CVE fix in 5.0.1 related to XFixesGetCursorImage() (which is the only function from XFixes scrot uses). And it doesn't seem like version 6.0.0 broke anything regarding XFixesGetCursorImage() so go ahead and settle on v5.0.1 for XFixes.


Xinerama v1.1.3 was released 10 years ago and seems to contain a CVE fix. So let's go for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related
Projects
None yet
Development

No branches or pull requests

2 participants