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

New package: flipper #210

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

New package: flipper #210

wants to merge 6 commits into from

Conversation

darkbasic
Copy link
Contributor

@darkbasic darkbasic commented Mar 9, 2023

I've taken inspiration from the element-desktop ebuild, but instead of going for the hacky way I decided I wanted to seamlessly build for ppc64 like if it was a supported architecture. So I've created an app-builder ebuild because no ppc64 binary is being bundled with the npm package and I also created some patches to bake ppc64 support into builder-util. I've also had to patch flipper itself because the build scripts were broken and we were unlucky enough to expose the bug. Note that we can't use electron's node because node 18 is mandatory, otherwise it will fail to generate the app.asar.
Use the online build (default) for the moment because I have no idea how to generate the SRC_URI for offline usage.

Running the same exact commands in a normal shell results in a perfectly working flipper, but doing so using the ebuild results in the following error:

⚙️  Compiling main bundle...
2023-03-09T23:09:29,723: [cli] Watchman is running at a lower than normal priority. Since that results in poor performance that is otherwise very difficult to trace, diagnose and debug, Watchman is refusing to start.
Fatal error detected at:

/usr/lib64/libfolly.so.0.58.0-dev(+0x3d271f)[0x3fffbb5c271f]
/usr/lib64/libfolly.so.0.58.0-dev(_ZN5folly10symbolizer17getStackTraceSafeEPmm+0x33)[0x3fffbb5c28c3]
/usr/lib64/libfolly.so.0.58.0-dev(_ZN5folly10symbolizer21SafeStackTracePrinter15printStackTraceEb+0x6b)[0x3fffbb5c5c9b]
watchman(+0x261013)[0x10d9d1013]
watchman(+0x261663)[0x10d9d1663]
watchman(+0x25a4ff)[0x10d9ca4ff]
watchman(+0x14a19f)[0x10d8ba19f]
watchman(+0x148497)[0x10d8b8497]
watchman(+0x1492c7)[0x10d8b92c7]
watchman(+0x31e9f)[0x10d7a1e9f]
/usr/lib64/libc.so.6(+0x24c23)[0x3fffba774c23]
/usr/lib64/libc.so.6(__libc_start_main+0x1af)[0x3fffba774e6f]
(safe mode, symbolizer not available)

Watchman:  watchman --no-pretty get-sockname returned with exit code=1, signal=null, stderr= 2023-03-09T23:09:29,723: [cli] Watchman is running at a lower than normal priority. Since that results in poor performance that is otherwise very difficult to trace, diagnose and debug, Watchman is refusing to start.
Fatal error detected at:
/usr/lib64/libfolly.so.0.58.0-dev(+0x3d271f)[0x3fffbb5c271f]
/usr/lib64/libfolly.so.0.58.0-dev(_ZN5folly10symbolizer17getStackTraceSafeEPmm+0x33)[0x3fffbb5c28c3]
/usr/lib64/libfolly.so.0.58.0-dev(_ZN5folly10symbolizer21SafeStackTracePrinter15printStackTraceEb+0x6b)[0x3fffbb5c5c9b]
watchman(+0x261013)[0x10d9d1013]
watchman(+0x261663)[0x10d9d1663]
watchman(+0x25a4ff)[0x10d9ca4ff]
watchman(+0x14a19f)[0x10d8ba19f]
watchman(+0x148497)[0x10d8b8497]
watchman(+0x1492c7)[0x10d8b92c7]
watchman(+0x31e9f)[0x10d7a1e9f]
/usr/lib64/libc.so.6(+0x24c23)[0x3fffba774c23]
/usr/lib64/libc.so.6(__libc_start_main+0x1af)[0x3fffba774e6f]
(safe mode, symbolizer not available)

/var/tmp/portage/dev-util/flipper-0.183.0/work/flipper-0.183.0/desktop/node_modules/metro-hermes-compiler/src/emhermesc.js:77
          throw ex;
          ^

Error: watchman --no-pretty get-sockname returned with exit code=1, signal=null, stderr= 2023-03-09T23:09:29,723: [cli] Watchman is running at a lower than normal priority. Since that results in poor performance that is otherwise very difficult to trace, diagnose and debug, Watchman is refusing to start.
Fatal error detected at:
/usr/lib64/libfolly.so.0.58.0-dev(+0x3d271f)[0x3fffbb5c271f]
/usr/lib64/libfolly.so.0.58.0-dev(_ZN5folly10symbolizer17getStackTraceSafeEPmm+0x33)[0x3fffbb5c28c3]
/usr/lib64/libfolly.so.0.58.0-dev(_ZN5folly10symbolizer21SafeStackTracePrinter15printStackTraceEb+0x6b)[0x3fffbb5c5c9b]
watchman(+0x261013)[0x10d9d1013]
watchman(+0x261663)[0x10d9d1663]
watchman(+0x25a4ff)[0x10d9ca4ff]
watchman(+0x14a19f)[0x10d8ba19f]
watchman(+0x148497)[0x10d8b8497]
watchman(+0x1492c7)[0x10d8b92c7]
watchman(+0x31e9f)[0x10d7a1e9f]
/usr/lib64/libc.so.6(+0x24c23)[0x3fffba774c23]
/usr/lib64/libc.so.6(__libc_start_main+0x1af)[0x3fffba774e6f]
(safe mode, symbolizer not available)

    at ChildProcess.<anonymous> (/var/tmp/portage/dev-util/flipper-0.183.0/work/flipper-0.183.0/desktop/node_modules/fb-watchman/index.js:198:18)
    at ChildProcess.emit (node:events:513:28)
    at ChildProcess.emit (node:domain:489:12)
    at maybeClose (node:internal/child_process:1091:16)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:302:5)
Emitted 'error' event on Client instance at:
    at spawnError (/var/tmp/portage/dev-util/flipper-0.183.0/work/flipper-0.183.0/desktop/node_modules/fb-watchman/index.js:170:10)
    at ChildProcess.<anonymous> (/var/tmp/portage/dev-util/flipper-0.183.0/work/flipper-0.183.0/desktop/node_modules/fb-watchman/index.js:198:7)
    at ChildProcess.emit (node:events:513:28)
    [... lines matching original stack trace ...]
    at Process.ChildProcess._handle.onexit (node:internal/child_process:302:5)

Node.js v18.14.2

Could you give it a try on your system? Do you have any idea what's going on? Surely some sandbox sh*t with watchman but I'm really too tired to further look into it now.
If we can manage to get the ebuild working I will backport the new ppc64 build method into the element-desktop ebuild and I will add the electron wrapper to both as well.

@darkbasic
Copy link
Contributor Author

It turns out that, at least on ppc64, if you have dev-util/watchman installed it will try to use that and it will throw that error (presumably if you are in a sandox, since from a normal shell it will work). I don't want to add a conflicts with dev-util/watchman because a simple mv /usr/bin/watchman /usr/bin/watchman.temp is enough to fix the build and later you can always rename it back, but still that's VERY annoying. Ideas?

@darkbasic
Copy link
Contributor Author

darkbasic commented Mar 10, 2023

There was a sneaky preinstall script which was hiding an otherwise disabled watchman config. Unfortunately that wasn't enough, so I started looking for watchman references all over the code and got rid of them. Still not enough, apparently in a purely node-ish fashion some deep deep dependency was still starting the damn thing, so I decided to take care of the problem at the root and I've crafted a very elaborate patch. Done. Adios :)

@darkbasic
Copy link
Contributor Author

I've added the electron wrapper as well. @PF4Public please let me know how to generate the SRC_URI with all the node deps for offline usage and I'll mark this as ready for review.

@darkbasic
Copy link
Contributor Author

In case that will get merged we will be able to drop app-builder one day: gentoo/gentoo#30029

…n everywhere, turns out flipper needs it at runtime
@darkbasic
Copy link
Contributor Author

Turns out if you use nukes to kill mosquitoes it can backfire: flipper could potentially make use of watchman at runtime and the previous patch prevented that. So instead of nuking it altogether I spent a couple more hours debugging it and I ended up disabling it in the metro bundler.

@PF4Public
Copy link
Owner

PF4Public commented Mar 11, 2023

Sorry, couldn't reply earlier.

Wow, that's a lot!

I suppose, you've solved the watchman issue, but here are some things for consideration

https://github.com/facebook/watchman/blob/1520533f83f02412999517ad4b49e9ffaebe7e7f/watchman/main.cpp#L95-L99

It looks for min_acceptable_nice_value in options. Could it be useful?

And here it calls a system watchman:
https://github.com/facebook/watchman/blob/1520533f83f02412999517ad4b49e9ffaebe7e7f/watchman/node/index.js#L29-L32

Maybe watchmanBinaryPath could be overriden?

seamlessly build for ppc64 like if it was a supported architecture.

This could be expanded to other arches instead of downloading third-party binaries. What worries me is that it pulls in go. That's a huge dependency. So if you add dependency on app-builder, you'd better keep the old "hacky" way and make them if/else cases in element based on say app-builder USE flag.

please let me know how to generate the SRC_URI with all the node deps for offline usage

Here is the bash excerpt that I've cut from my script with mostly all the unrelated things removed. Might not work on the first try: at least you need to add additional place for searching for yarn.lock in locks variable. The result is in /tmp/out_deps

tmp_dir=$(mktemp -d)
# echo $tmp_dir
# tmp_dir="/tmp/tmp.jeKig0qsTv"
pushd "$tmp_dir" >/dev/null
	wget $url
	unzip -q *.zip
	pushd */ >/dev/null
			if [ -d "extensions" ]; then
				mapfile -t locks < <(find extensions build -name yarn.lock 2>/dev/null)
			fi

			if [ -d "node_modules" ]; then
				mapfile -t locks < <(find node_modules -name yarn.lock 2>/dev/null)
			fi

			locks+=('yarn.lock')

			urls=()

			echo "Locks: ${locks[@]}"

			for i in "${locks[@]}"; do
				mapfile -t urls_temp < <(grep resolved $i | sed -e 's/.*\"\(https[^#"]\+\).*/\1/')
				urls+=("${urls_temp[@]}")
			done

			declare -A b

			for i in "${urls[@]}"; do b["$i"]=1; done

			cat /dev/null >/tmp/out_deps

			for i in "${!b[@]}"; do
				if [[ $i =~ ^.*(@[^/]+)/.*/([^/]+)$ ]]; then
					out="$i -> ${BASH_REMATCH[1]}-${BASH_REMATCH[2]}"
				else
					out=$i
				fi
				echo -e "\t$out" >>/tmp/out_deps
			done

			sort -o /tmp/out_deps /tmp/out_deps
	popd >/dev/null
popd >/dev/null
rm -rf $tmp_dir

I've rewritten that in Perl for the bot since then, but I suppose Bash is what you're after.

@darkbasic
Copy link
Contributor Author

It looks for min_acceptable_nice_value in options. Could it be useful?

Oh that might indeed be the issue: I completely forgot I have configured portage to use an ultra low priority and that could be why it fails (in fact I've tried to disable the sandbox and it keeps failing). Unfortunately a high niceness is a perfectly sane value for portage so we can't just assume that users won't run it at low prio.

Maybe watchmanBinaryPath could be overriden?

That's basically what my first patch was doing, but the same library gets bundled into the app.asar and that would prevent Flipper from being able to use it (which is exactly what it's doing if watchman is installed). The solution I ended up with is the best of both worlds because it surgically disables watchman just for the metro bundling.

This could be expanded to other arches instead of downloading third-party binaries. What worries me is that it pulls in go. That's a huge dependency. So if you add dependency on app-builder, you'd better keep the old "hacky" way and make them if/else cases in element based on say app-builder USE flag.

Of course we can do the same for whatever arch we want (but would be pointless unless electron builds on that arch). I personally don't like the fact that npm handles binaries and I would rather prefer to have binary packages handled by portage (I don't like using binaries at all in Gentoo, but that's personal preference). The main disadvantage of the "hacky" way (and the main reason why I wanted to part with it) is that it requires additional efforts to figure out how to package each and every application instead of simply leveraging the existing build system. Instead of doing so it would probably be better to additionally provide an app-builder binary for ppc64, either as another ebuild or even as its own npm package. With the latter we could simply add a yarn resolutions entry to override the official app-builder package with our own that also contains binaries for other architectures. Actually I really like this idea, it might even be worth providing ppc64 electron packages on npm: if we do so chances are I might even succeed to upstream official ppc64 support in builder-util!
I'm mentioning @TheCodex6824 since he's the one who figured out the original workaround and he uses ppc64 as well.

@TheCodex6824
Copy link
Contributor

Nice work - just gave this a try and it seems to work fine on ppc64le with build-online electron-21, and I don't have dev-util/watchman installed. Only issue I could find was tc-endian not being found, but everything seemed to work anyway:

/var/tmp/portage/dev-util/app-builder-4.1.2/temp/environment: line 663: tc-endian: command not found

It looks like go-module.eclass is not inheriting toolchain-funcs which tc-endian lives in, but I'm not sure if that is the issue or something is just broken on my system.

As for getting app-builder working - the only potential issue I can see is with builder-util trying to download a 7zip binary (specifically 7za) if something tries to compress with that format. Is it possible to replace that with the system 7zip like you did with app-builder?

@darkbasic
Copy link
Contributor Author

As for getting app-builder working - the only potential issue I can see is with builder-util trying to download a 7zip binary (specifically 7za) if something tries to compress with that format. Is it possible to replace that with the system 7zip like you did with app-builder?

I already did so and it works well, but I've decided to leave it out because we won't compress/decompress anything in this ebuild yet.

@PF4Public
Copy link
Owner

Of course we can do the same for whatever arch we want (but would be pointless unless electron builds on that arch). I personally don't like the fact that npm handles binaries and I would rather prefer to have binary packages handled by portage (I don't like using binaries at all in Gentoo, but that's personal preference).

I meant exactly this. Expand app-builder usage for already existing arches behind the flag. For those, who'd better build binaries locally in exchange for added go dependency.

It looks for min_acceptable_nice_value in options. Could it be useful?

Oh that might indeed be the issue: I completely forgot I have configured portage to use an ultra low priority and that could be why it fails (in fact I've tried to disable the sandbox and it keeps failing). Unfortunately a high niceness is a perfectly sane value for portage so we can't just assume that users won't run it at low prio.

My idea was that this setting could be adjusted somehow to use system watchman if available.

@PF4Public
Copy link
Owner

By the way, @darkbasic Have you tried export ELECTRON_SKIP_BINARY_DOWNLOAD=1 instead of dev-util/flipper/files/dont-download-electron.patch?

@darkbasic
Copy link
Contributor Author

darkbasic commented Mar 15, 2023

I'm pretty sure I did, but I will double check.

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

Successfully merging this pull request may close these issues.

3 participants