-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
postgresql: Remove dev dependencies from closure #179962
Conversation
+ echo "#define INCLUDEDIRSERVER \"/dev/null/include/server\"" >>$@ | ||
echo "#define LIBDIR \"$(libdir)\"" >>$@ | ||
echo "#define PKGLIBDIR \"$(pkglibdir)\"" >>$@ | ||
echo "#define LOCALEDIR \"$(localedir)\"" >>$@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could shave 16M more by removing man
and doc
output references.
79cf3ff
to
516faaf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or are the files copied into an output?
pkgs/servers/sql/postgresql/patches/hardcode-pg_config-paths.patch
Outdated
Show resolved
Hide resolved
pkgs/servers/sql/postgresql/patches/hardcode-pg_config-paths.patch
Outdated
Show resolved
Hide resolved
pkgs/servers/sql/postgresql/patches/hardcode-pg_config-paths.patch
Outdated
Show resolved
Hide resolved
516faaf
to
4461a4d
Compare
@ofborg build postgresql_10 postgresql_11 postgresql_12 postgresql_13 postgresql_14 |
4461a4d
to
67b91ed
Compare
@ofborg build postgresql_10 postgresql_11 postgresql_12 postgresql_13 postgresql_14 |
67b91ed
to
cbca884
Compare
2894ece
to
beaa49a
Compare
I'll try to take a look this weekend. |
`pg_config` stores configure flags into the library/program for debugging or something. Since `PKG_CONFIG_PATH` somehow ends up in there, `dev` outputs of dependencies get pulled into the runtime closure. Let’s clear the paths in the variable to reduce the closure size. Reduces size of out: 287.8M → 236.5M
No closure reduction this time since there are still more references.
Looks like the current reduction of So unless there were some regressions, the commit splitting the |
d07037b
to
7ab57c1
Compare
|
||
configdata[i].name = pstrdup("PGXS"); | ||
- get_pkglib_path(my_exec_path, path); | ||
+ strlcpy(path, "/dev/null", sizeof(path)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale in here to make this /dev/null
first and then replace that with @dev@
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC pg_config.c
is installed to dev output but config_info.c
ends up as a part of out
.
} | ||
|
||
+static char* | ||
+get_config_val(ConfigData configdata) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also why is this passed by reference here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is passed by value, is not it? We could pass a pointer to it if we want to avoid copying the whole struct but I do not think this is worth it – the functions will probably only be called at extension build time and compiler might inline the function anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meh, I mixed them up, I meant pass-by-value and wanted to ask why 🙃
- echo "#define INCLUDEDIR \"$(includedir)\"" >>$@ | ||
- echo "#define PKGINCLUDEDIR \"$(pkgincludedir)\"" >>$@ | ||
- echo "#define INCLUDEDIRSERVER \"$(includedir_server)\"" >>$@ | ||
+ echo "#define INCLUDEDIR \"/dev/null/include\"" >>$@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this break with e.g. https://github.com/postgres/postgres/blob/aa210e0c121eb8f58c86d4fcc833a5a6fbb6f5a9/src/interfaces/ecpg/preproc/ecpg.c#L256 since get_include_path
uses INCLUDEDIR
generated by this makefile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will return the wrong value but the assumption is that nothing uses the function for anything other than printing it in some build information pages.
@@ -87,7 +99,6 @@ let | |||
"--with-libxml" | |||
"--with-icu" | |||
"--sysconfdir=/etc" | |||
"--libdir=$(lib)/lib" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm probably missing something, but how are .so
files moved to the lib
output now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We previously passed setOutputFlags = false;
so we had to set it explicitly. Now that I removed it, multiple-outputs hook passes this for us.
@@ -87,6 +87,10 @@ let | |||
tomli | |||
]; | |||
|
|||
buildInputs = [ | |||
postgresql | |||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate why this is now needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a proper place for this as a library dependency. Ideally, we would remove it from nativeBuildInputs
but that will probably not work until psycopg/psycopg2#1001. cc @SuperSandro2000 who made this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, IIRC they use pg_config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment.
AC_CONFIG_AUX_DIR(config) | ||
AC_PREFIX_DEFAULT(/usr/local/pgsql) | ||
-AC_DEFINE_UNQUOTED(CONFIGURE_ARGS, ["$ac_configure_args"], [Saved arguments from configure]) | ||
+AC_DEFINE_UNQUOTED(CONFIGURE_ARGS, ["`echo "$ac_configure_args" | sed -E 's~/nix/store/.{32}-~/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-~g'`"], [Saved arguments from configure]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's happening to e.g. tzdata which is also part of the configure flags and a store path? This means that every single reference in configureFlags
silently gets discarded, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. But these should just be for informational reports.
Now that we are done with it, we should keep it, to keep out dev things from the final system. |
Weirdly,
|
I think we can just ignore those. There are many other tests that pass and from the description they read like they can't be that stable. |
The package contains some files only necessary for linking other software against PgSQL’s libraries. Those files include compiler flags that reference `dev` outputs of other libraries, unnecessarily increasing the runtime closure. Let’s move those files to a `dev` output, reducing the runtime closure of `out`. We also need to clear out some paths hardcoded into the libs to avoid a dependency cycle between the `dev` and `out` outputs. This includes the path to PGXS files for the `libpgcommon` and `postgres` server (used e.g. by `SELECT pg_config()` query); we restore the path explicitly for `pg_config` program. The `out` output will be the root of the graph. This further reduces the closure size of out from 236.5M to 235.5M.
57c494f
to
8f21582
Compare
Superseded by #294504 |
This was supposed to happen in NixOS#294504, but the commit was accidentally left out when splitting off some libpq-related changes. Originated in NixOS#179962, by Sandro. Co-authored-by: Sandro Jäckel <sandro.jaeckel@gmail.com> Co-authored-by: Wolfgang Walther <walther@technowledgy.de>
Description of changes
Reduces size of out: 327.3M → 243.6M
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes