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

folly: split outputs to reduce closure sizes w/ cmake fixup #278643

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

bryango
Copy link
Member

@bryango bryango commented Jan 4, 2024

Description of changes

This shaves off a 100MiB+ boost.dev dependence from folly.out, which would benefit all downstream packages that dynamically link to folly.

Before:

$ nix path-info -rsh nixpkgs#watchman | sort -hk2 -r | head -5
/nix/store/h1r04r2cay50i5k96b3miw27v8r9zwkr-boost-1.81.0-dev       	 145.1M
/nix/store/l77dphhwcw7j5k5kqxw1rqxbvhszgaw7-icu4c-73.2             	  37.7M
/nix/store/9y8pmvk8gdwwznmkzxa6pwyah52xy3nk-glibc-2.38-27          	  28.8M
/nix/store/9w9ndv9n74qid65gglh9h127mh4ndba8-boost-1.81.0           	  14.0M
/nix/store/bjmm3lshlbv99rx8mldnbb1yzhl2kya2-folly-2023.02.27.00    	  13.4M

After:

$ nix path-info -rsh .#watchman | sort -hk2 -r | head -5
/nix/store/l77dphhwcw7j5k5kqxw1rqxbvhszgaw7-icu4c-73.2             	  37.7M
/nix/store/9y8pmvk8gdwwznmkzxa6pwyah52xy3nk-glibc-2.38-27          	  28.8M
/nix/store/9w9ndv9n74qid65gglh9h127mh4ndba8-boost-1.81.0           	  14.0M
/nix/store/dghjv6hfz0s0z4kffa5ahyw2mhp79215-gcc-12.3.0-lib         	   7.5M
/nix/store/xqidf03vlwb6paj02s23j8g77f1wag0k-watchman-2023.08.14.00 	   6.7M

Fixes #248660.

Technical issues

Upstream's cmake configuration is not suitable for split packages, and some tweaks are needed for it to work; these are taken from:

See the tracking issue:

Update: turns out this will be a mass rebuild, re-targeted towards staging.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@bryango
Copy link
Member Author

bryango commented Jan 4, 2024

Turns out this is a mass rebuild 😲 and I have re-targeted towards staging. I have never gone through the staging workflow, so please advise what to do 🥺

@bryango bryango changed the base branch from master to staging January 4, 2024 07:33
@bryango bryango marked this pull request as ready for review January 4, 2024 10:36
@bryango bryango marked this pull request as draft January 4, 2024 10:50
@bryango bryango force-pushed the folly-multi-outputs branch 6 times, most recently from c3de49f to c287d7a Compare January 5, 2024 04:36
@bryango bryango changed the title folly: split outputs to reduce closure sizes folly: split outputs to reduce closure sizes w/ cmake fixup Jan 5, 2024
@bryango bryango marked this pull request as ready for review January 5, 2024 05:46
@bryango
Copy link
Member Author

bryango commented Jan 5, 2024

@dcecile @andrewhamon @aaronjheng This is ready for review. I don't know much about folly at all; I am just a downstream user of watchman trying to fix my own complaint:

I have pinged some people who have recently contributed watchman and/or folly. Sorry about the multiple force pushes as I struggled to fix the cmake issues, but I believe it is finally resolved!

@jonringer
Copy link
Contributor

@ofborg build watchman fbthrift wangle fizz

@bryango bryango requested a review from jonringer January 5, 2024 07:58
@bryango
Copy link
Member Author

bryango commented Jan 5, 2024

x86_64-darwin fails, but the failures seem to be some weird timeouts, and I am not sure if they are related to the changes here... I will try a build with github actions on darwin (355 derivations, may take a loooong time).

Update: oh I understand what is happening, we must rebuild stdenv-darwin, which depends on llvm, which depends on a bunch of crazy stuff... and it is failing on my github action.

This shaves off a 100MiB+ `boost.dev` dependence from `folly.out`, which
would benefit all downstream packages that dynamically link to folly.

Upstream's cmake configuration is not suitable for split packages, and
some tweaks are applied for it to work; these are taken from:

- NixOS@04384d5
- NixOS@8d712cc

See: https://github.com/jtojnar/cmake-snips
@bryango
Copy link
Member Author

bryango commented Jan 5, 2024

Rebasing to see whether we can have some new darwin caches.
@ofborg build watchman fbthrift wangle fizz

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

The linux versions of downsteam dependencies build, so I think this is good to go into staging. Please take look during the next staging-next cycle :)

Comment on lines +68 to +70
# see https://github.com/NixOS/nixpkgs/issues/144170
"-DCMAKE_INSTALL_INCLUDEDIR=include"
"-DCMAKE_INSTALL_LIBDIR=lib"
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice if they use GnuInstallDirs and not use bespoke re-implementations.... but facebook does what facebook does....
Would also simplify the .pc (e.g. https://github.com/protocolbuffers/protobuf/blob/main/cmake/protobuf.pc.cmake)

Also, they pretty much don't look at public feedback: facebook/folly#1715

Copy link
Member Author

@bryango bryango Jan 6, 2024

Choose a reason for hiding this comment

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

facebook does what facebook does....

Yeah! They also use a homemade build system with thousands of LoCs to wrap cmake, which is impossible to comprehend for amateurs like me... Oh, and they actually discourage packaging. So there is nothing much we could do... I will try to raise an issue upstream and see if anything happens.

@jonringer jonringer merged commit b5eb666 into NixOS:staging Jan 5, 2024
22 of 24 checks passed
@bryango
Copy link
Member Author

bryango commented Jan 6, 2024

Checked: watchman builds fine on top of staging!

Here are some outdated confusion about possible broken builds...

I found out that darwin watchman already fails on HEAD^ (10b0f19). Namely, it was already failing on the parent of this PR, so this is unrelated to the changes made here (not my fault, yay haha). I will see what I can do to fix it (probably not much, I don't have a mac, but I will try to abuse my github actions).

Some logs from building on HEAD^:

watchman> Running phase: buildPhase
watchman> build flags: -j4 SHELL=/nix/store/5dbzggaw2194zb986zg9yzyjl14kxjra-bash-5.2-p21/bin/bash
watchman> [  0%] Building CXX object CMakeFiles/err.dir/watchman/Poison.cpp.o
watchman> [  1%] Building pywatchman
watchman> [  2%] Building CXX object CMakeFiles/log.dir/watchman/PubSub.cpp.o
watchman> [  3%] Building C object CMakeFiles/wildmatch.dir/watchman/thirdparty/wildmatch/wildmatch.c.o
watchman> Traceback (most recent call last):
watchman>   File "/tmp/nix-build-watchman-2023.08.14.00.drv-0/source/watchman/python/setup.py", line 12, in <module>
watchman>     from setuptools import Extension, setup
watchman> ModuleNotFoundError: No module named 'setuptools'
watchman> During handling of the above exception, another exception occurred:
watchman> Traceback (most recent call last):
watchman>   File "/tmp/nix-build-watchman-2023.08.14.00.drv-0/source/watchman/python/setup.py", line 14, in <module>
watchman>     from distutils.core import Extension, setup
watchman> ModuleNotFoundError: No module named 'distutils'
watchman> make[2]: *** [CMakeFiles/pybuild.dir/build.make:79: build/pytimestamp] Error 1
watchman> make[1]: *** [CMakeFiles/Makefile2:216: CMakeFiles/pybuild.dir/all] Error 2
watchman> make[1]: *** Waiting for unfinished jobs....
watchman> [  3%] Building CXX object CMakeFiles/log.dir/watchman/LogConfig.cpp.o
watchman> [  3%] Linking C static library libwildmatch.a
watchman> [  3%] Built target wildmatch
watchman> [  4%] Building CXX object CMakeFiles/log.dir/watchman/Logging.cpp.o
watchman> [  4%] Building CXX object CMakeFiles/log.dir/watchman/portability/Backtrace.cpp.o
watchman> [  5%] Building CXX object CMakeFiles/err.dir/watchman/root/warnerr.cpp.o
watchman> [  6%] Linking CXX static library liblog.a
watchman> /nix/store/ybd6k83z67yjz427d4qji5i7dycr7azi-clang-wrapper-16.0.6/bin/ranlib: file: liblog.a(Backtrace.cpp.o) has no symbols
watchman> [  6%] Built target log
watchman> [  6%] Linking CXX static library liberr.a
watchman> [  6%] Built target err
watchman> make: *** [Makefile:146: all] Error 2

Update: watchman builds fine on 4a88766, so something in:

broke watchman on darwin, but it is possible that it has already been fixed in master or staging-next. Trying to build on the latest staging (probably not gonna work, 300+ derivations).

Update: ok, it seems that python is invoked without the needed setuptools and distutils. Somehow, python sneaks into the build env (trying to reproduce this); it is not supposed to be there! In the last successful build 4a88766, there is no python in path (reproduced here).

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

Successfully merging this pull request may close these issues.

watchman: huge closure with boost.dev dependence
2 participants