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

[staging] mariadb-connector-c: reduce closure size #71860

Closed
wants to merge 3 commits into from

Conversation

jonringer
Copy link
Contributor

Motivation for this change

noticed some -dev paths in the tree when reviewing another PR

[13:49:30] jon@jon-workstation ~/projects/nixpkgs (master)
$ nix path-info -Sh ./result
/nix/store/k4ba1iabxgvkxd1bm4bnx5bpva5rihbi-mariadb-connector-c-3.1.2     48.5M
[13:46:18] jon@jon-workstation ~/projects/nixpkgs (reduce-mariadb-connector)
$ nix path-info -Sh ./result
/nix/store/xkw305kzcprh2x8d81h8irwg8i9pfqrg-mariadb-connector-c-3.1.2     36.7M
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@jonringer
Copy link
Contributor Author

I feel like this should not be causing this many rebuilds

@FRidh FRidh added this to New in Staging Oct 24, 2019
@FRidh FRidh moved this from New to Needs review in Staging Oct 24, 2019
@aanderse
Copy link
Member

cc @Izorkin

@Izorkin
Copy link
Contributor

Izorkin commented Oct 25, 2019

ln -sv mariadb $out/lib/mysql and ln -sv libmariadbclient.a $out/lib/mariadb/libmysqlclient.aneed to correct normal build others packages.
@GrahamcOfBorg test prosodyMysql

@Izorkin
Copy link
Contributor

Izorkin commented Oct 25, 2019

With this update error:

building '/nix/store/x6brfn2qj50svxmjnn8x9ynp54z107mn-lua5.2-luadbi-mysql-0.7.2-1.drv'...
building '/nix/store/pc5a6ygripsrgz6c3cm3ykhyzji7p00f-perl5.30.0-DBD-mysql-4.050.drv'...
unpacking sources
unpacking source archive /nix/store/yfyjaacv1q6cdvf48i9p16phb0asqkiq-DBD-mysql-4.050.tar.gz
source root is DBD-mysql-4.050
setting SOURCE_DATE_EPOCH to timestamp 1547024428 of file DBD-mysql-4.050/META.json
patching sources
configuring
unpacking sources
unpacking source archive /nix/store/73x4fh5lalcxksi767sa58npdsq9bycw-luadbi-mysql-0.7.2-1.src.rock
patching ./lib/DBD/mysql.pm...
patching ./t/56connattr.t...
Done. You may now enter directory
luadbi-mysql-0.7.2-1/luadbi
and type 'luarocks make' to build.
source root is ./luadbi-mysql-0.7.2-1/luadbi
setting SOURCE_DATE_EPOCH to timestamp 1555110721 of file ./luadbi-mysql-0.7.2-1/luadbi/vc++/luadbi.sln
patching sources
configuring
building
patching script interpreter paths in .
installing
Error: Could not find header file for MYSQL
  No file mysql.h in /nix/store/iagmkzffrbp5cadzrmj5gmb5wx7rmx9v-mariadb-connector-c-3.1.2/include/mysql
You may have to install MYSQL in your system and/or pass MYSQL_DIR or MYSQL_INCDIR to the luarocks command.
Example: luarocks install luadbi-mysql MYSQL_DIR=/usr/local
builder for '/nix/store/x6brfn2qj50svxmjnn8x9ynp54z107mn-lua5.2-luadbi-mysql-0.7.2-1.drv' failed with exit code 1

@jonringer
Copy link
Contributor Author

I'll revert the symlink removal

@jonringer
Copy link
Contributor Author

@GrahamcOfBorg test prosodyMysql

@jonringer
Copy link
Contributor Author

@GrahamcOfBorg eval

@jonringer jonringer changed the base branch from master to staging October 25, 2019 17:33
@jonringer
Copy link
Contributor Author

jonringer commented Oct 25, 2019

switched the base, as this causes a lot of rebuilds

@jonringer jonringer changed the title mariadb-connector-c: reduce closure size [staging] mariadb-connector-c: reduce closure size Oct 25, 2019
@ofborg ofborg bot added the 6.topic: vim label Oct 25, 2019
@ofborg ofborg bot requested review from lovek323 and edolstra October 25, 2019 17:52
@Izorkin
Copy link
Contributor

Izorkin commented Oct 29, 2019

After this change, only file is deleted the ./nix-support/propagated-build-inputs. Size reduced by 183 bytes.

dr-xr-xr-x 2 root root   3 янв  1  1970 .
dr-xr-xr-x 6 root root   6 янв  1  1970 ..
-r--r--r-- 9 root root 183 янв  1  1970 propagated-build-inputs

It seems that the propagated-buildinputs file indicates where to look for the necessary dependencies to build other packages. Is it worth it to delete? (sorry, bad english)

@jonringer
Copy link
Contributor Author

it's still significant:

staging:

$ nix path-info -Sh ./result
/nix/store/cx2hq38vbl65b924iqrj50g8faixq253-mariadb-connector-c-3.1.4     48.5M

pr:

$ nix path-info -Sh ./result
/nix/store/0xj2649x88krvaaxvkmw3n7fi1qpr2jv-mariadb-connector-c-3.1.4     36.9M

@aanderse
Copy link
Member

@jonringer are we good to go on this?

@FRidh
Copy link
Member

FRidh commented Nov 12, 2019

have all the direct reverse dependencies been built? I am asking because they may be effected by the removal of propagated build inputs.

@globin
Copy link
Member

globin commented Nov 12, 2019

Have you looked at the pkgconfig definitions if these deps are not needed in dependants?

@jonringer
Copy link
Contributor Author

This could still probably use some love, I'll try to get down to 0 regressions

@jonringer
Copy link
Contributor Author

rebased on top of current staging

@jonringer
Copy link
Contributor Author

now i remember why I grew tired of this... xD

[0/7919 built, 24/1943/2928 copied (8514.0/28246.9 MiB), 6281.4/13202.1 MiB DL]

@Br1ght0ne
Copy link
Member

6. topic: closure-size label may be relevant.

@Izorkin
Copy link
Contributor

Izorkin commented Nov 12, 2019

Packages that use libmysqlclient - grass kexi libreoffice glsurf kodi gerbil neko urweb haskellPackages.mysql php72 php73 cppdb gdal gdal_2 libagar libdbiDriversBase libgda redland opendbx poco tntdb unixODBCDrivers.mariadb wt3 wt4 lispPackages.clsql lua51Packages.luadbi-mysql lua52Packages.luadbi-mysql lua53Packages.luadbi-mysql luajitPackages.luadbi-mysql ocamlPackages.ocaml_mysql perlPackages.DBDmysql purePackages.glpk pythonPackages.MySQL_python pythonPackages.mysqlclient rPackages.RMySQL sysbench tora clickhouse slurm freeradius lighttpd dovecot opensmtpd postfix zabbix uwsgi mydumper kea mailutils thc-hydra collectd
Maybe something is superfluous, or something is missing.
There are also packages in which mysql support is disabled by default.

@jonringer
Copy link
Contributor Author

the problem with propagatedBuildInputs is that downstream dependencies (past direct dependencies) can still receive buildInputs

@globin
Copy link
Member

globin commented Nov 12, 2019

The thing is that if mariadb-connector-c requires dependants to have zlib and openssl in the buildInputs then we can have them in propagatedBuildInputs and we just require every dependant to have extra code and don't save anything?

@globin
Copy link
Member

globin commented Nov 12, 2019

Yeah, I just checked:

$ cat result-dev/lib/pkgconfig/libmariadb.pc
...
Libs.private: -lz -ldl -lm -lpthread -lssl -lcrypto

This doesn't make any sense, sorry...

@jonringer
Copy link
Contributor Author

jonringer commented Nov 12, 2019

on the flip side, if a package wants to use a different version of openssl, then I'm not sure if they will get the one they explicitly wanted, or the one that was propagated?

I may not fully understand the circumstances in which mariadb-connector-c can to be used, but seems weird to me if I'm implicitly receiving dependencies like zlib or openssl, even though I don't have them as a direct dependency

@globin
Copy link
Member

globin commented Nov 13, 2019

Then you'd most probably have to override for mariadb-connector-c too.

The pkgconfig line explicitly says that you, as a downstream user of the library, have to make it possible to link to zlib and openssl.

@jonringer
Copy link
Contributor Author

I'm thoroughly confused as to why a package has to receive zlib and openssl pkgconfig inputs from mariadb-connector-c.

But I've also lost all interesting in saving 12MB in closure size for a package I don't normally use.

@jonringer jonringer closed this Nov 13, 2019
Staging automation moved this from Needs review to Done Nov 13, 2019
@jonringer jonringer deleted the reduce-mariadb-connector branch November 13, 2019 01:52
@jonringer
Copy link
Contributor Author

From a correctness standpoint, propagatedBuildInputs isn't what you want when cross compiling, as it will propagate openssl and zlib of a different architecture

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Staging
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants