ImperatorStorm / PKGBUILDs

PKGBUILDs for packages I maintain
4 stars 1 forks source link

Use brand new PKGBUILD for Aseprite Package #1

Closed ISSOtm closed 2 years ago

ISSOtm commented 2 years ago

This attempts to fix the old one's jank.

TODOs:

(Aside: if you'd like, I can be a co-maintainer of the AUR package as well.)

ISSOtm commented 2 years ago

Findings about build deps:

ImperatorStorm commented 2 years ago

Add yourself on as a Maintainer and I'll merge.

ISSOtm commented 2 years ago

Thank you very much! Do we want to merge with the first post's TODOs still unresolved?

ImperatorStorm commented 2 years ago

Now that you mention it,

Not all dependencies checked out by git-sync-deps are necessary, since a lot of shared libs are used. Figure out a way to only sync what we need?

This should be done before merge Everything else was either already fixed or is fine.

Perhaps patching DEPS to only grab the dependencies we need it to grab?

Alternatively, manually sync the deps?

ISSOtm commented 2 years ago

I'm not sure which of these options is best. I'd rather sync them manually to avoid patching, but this might be cumbersome.

I'm also looking into dependencies again, especially icu which is not linked against despite skia_use_system_icu=true... tbh, it's quite hard to trace through all of the dependency chain.

ISSOtm commented 2 years ago

Ok, so, passing -v to ninja, and then grep -o -- '-I\s*../third_party/externals/\S*' | sort | uniq, it follows that only freetype and libgifcodec are being used, at least like this.

More investigation is required, since I remember not being able to compile without at least dng_sdk. But at least we should be able to get somewhere.

ISSOtm commented 2 years ago

Looking at src/skia/gn/skia.gni, it seems that the components that are being used are:

Component Used? Reason
expat No Using system's
fontconfig No Using system's... I think?
freetype Yes Using system's freetype2, but the bundled harfbuzz requires freetype (!)
harfbuzz Yes Can't use system's due to an API change, apparently, so must use bundled
gl No ??
icu No Using system's
libjpeg-turbo No Using system's
libpng No Using system's
libwebp No Using system's
piex No Must use bundled
x11 No ??
xps No ??
zlib No Using system's
dng_sdk Yes By default if using libjpeg-turbo and zlib... huh.
libgifcodec Yes By default if not using wuffs
sfntly No Enabled by default since we use icu, but Aseprite's build instructions disable it... except in the Linux example??

This table needs more work, esp. figuring out which libs are required for what (mainly, what file formats does Skia need to load? What is handled directly by Aseprite?) So consider the above a WIP. (Thankfully, we can already get rid of a lot of deps—many of them, such as imgui, are only used for building example programs, so we don't need them either.)

Looking at the various BUILD.gn, it seems that is_official_build sets use_system_*=true by default, so that would also simplify the gn config line.

I am also disabling skia_enable_pdf and skia_enable_skottie, as it seems that Aseprite uses neither. This should reduce the compile time.

ISSOtm commented 2 years ago

Ok, this should suffice for all the "cleanup" commits, and I've taken out 4 items from the OP's TODO list. I noticed that PDF support isn't actually disabled! Only some parts of it are, right now. I'll next look into axing that.

I'll check if e.g. disabling libpng causes Aseprite to fail linking, since I know for a fact that it expects to load and save PNG images. If it does, then we should be able to rely on disabling required features failing to build entirely; otherwise, we'd see missing features only have an impact at runtime.

Regardless, the bulk of the work should be done—I still have a lingering fear that I broke something with this amount of changes, but it compiles and runs fine for me—so this should be ready for review.

ImperatorStorm commented 2 years ago

Workflow failed, missing libglvnd makedependency, possibly runtime dep.

[289/912] compile ../src/gpu/gl/glx/GrGLMakeNativeInterface_glx.cpp
FAILED: obj/src/gpu/gl/glx/gpu.GrGLMakeNativeInterface_glx.o 
c++ -MD -MF obj/src/gpu/gl/glx/gpu.GrGLMakeNativeInterface_glx.o.d -DNDEBUG -DSK_R32_SHIFT=16 -DSK_GAMMA_APPLY_TO_A8 -DSKIA_IMPLEMENTATION=1 -DSK_GL -I.. -fstrict-aliasing -fPIC -O3 -fdata-sections -ffunction-sections -Wno-unused-parameter -std=c++17 -fno-exceptions -fno-rtti -c ../src/gpu/gl/glx/GrGLMakeNativeInterface_glx.cpp -o obj/src/gpu/gl/glx/gpu.GrGLMakeNativeInterface_glx.o
../src/gpu/gl/glx/GrGLMakeNativeInterface_glx.cpp:15:10: fatal error: GL/glx.h: No such file or directory
   15 | #include <GL/glx.h>
      |          ^~~~~~~~~~
ISSOtm commented 2 years ago

Something else: I'm noticing that the old PKGBUILD disabled the updater, but I currently am not (and thus it's enabled by default). Should we disable it?

ImperatorStorm commented 2 years ago

Yes, updater's likely gonna be broken with this packaging.

ISSOtm commented 2 years ago

New build fail, this time it's related to libwebp. I'll look at it tomorrow.

ImperatorStorm commented 2 years ago

Alright.

ISSOtm commented 2 years ago

Side note: I don't like that libwebp is being linked against dynamically by Skia, but not by the final executable. I'm a bit rusty on my static lib innards, but it seems possible that we'd be compiling Skia against the system headers, but linking against Aseprite's bundled lib.

ISSOtm commented 2 years ago

Yeah, I am afraid that's what's happening. The fix seems to be making the same lib consistently shared or static in both Skia and Aseprite.

For libwebp, we should make it shared by Aseprite as well; for freetype and harfbuzz, we should make Skia point to Aseprite's bundled headers (or the opposite, whichever works). I think that should work.

ImperatorStorm commented 2 years ago

pushed a commit based on https://patch-diff.githubusercontent.com/raw/aseprite/aseprite/pull/2523.patch, hopefully this fixes the libwebp issue.

Well, that's certainly an error

oh, its USE_SHARED_WEBP not USE_SHARED_LIBWEBP

ImperatorStorm commented 2 years ago

Also adding on pixman as a makedep, as we're using the shared library of it in aseprite.

ImperatorStorm commented 2 years ago

Ok ld failed to link, not sure what's going on there.

ld wants WebPDemux and WebPMux stuff, though it should have been built.

gif2webp or img2webp is required to get libwebpdemux

Hmm, -DWEBP_BUILD_GIF2WEBP=YES should be building it, not sure why ld's complaining.

2022-01-05T07:02:55.3726080Z CMake Warning: 2022-01-05T07:02:55.3727021Z Manually-specified variables were not used by the project: ... 2022-01-05T07:02:55.3729311Z WEBP_BUILD_GIF2WEBP 2022-01-05T07:02:55.3729704Z WEBP_BUILD_IMG2WEBP ...

Odd, looks like shared libwebp breaks this.

ISSOtm commented 2 years ago

That'd be normal, as libwebpmux and libwebpdemux are also part of libwebp.

ISSOtm commented 2 years ago

Seems like this works just fine, and I also managed to make harfbuzz and freetype shared :)

ISSOtm commented 2 years ago

Also adding on pixman as a makedep, as we're using the shared library of it in aseprite.

The shared lib doesn't appear to be used, since the LAF backend is Skia. I'll check if we can skip having it installed at all.

ImperatorStorm commented 2 years ago

Should we require clang? Skia's build instructions mention that performance may be significantly worse with non-Clang compilers

Regarding this, it would likely be best to require clang.

ISSOtm commented 2 years ago

Thing is, I don't know if it really holds true anymore. But I don't know how we'd bench performance either.

ImperatorStorm commented 2 years ago

Fair, might be best to not force clang then.

ISSOtm commented 2 years ago

Ah, I see that Skia's source has a bench subdirectory. Gimme a while to look at that.

ISSOtm commented 2 years ago

Looking more into it, it doesn't seem like it would make sense to bench Skia itself, but we'd be more interested in how Aseprite uses it. So I don't know. Maybe we should stick to the default for now (which can be configured in makepkg.conf, mind you), and pin a message in the AUR comments telling users to report any performance issues. Based on feedback we can then choose whether to require Clang.

ImperatorStorm commented 2 years ago

sounds good.

ImperatorStorm commented 2 years ago

Do we really conflict with skia-git? If so, that should be reinstated in the conflicts array

I can't imagine why we'd conflict with skia-git, probably shouldn't re-add it to conflicts.

ImperatorStorm commented 2 years ago

Run tests? E.g. LAF's, except that they fail to build due to an obscure linker error (looks like some static lib requires a dynamic lib that doesn't get -l'd?)

Should probs give it a try.

ISSOtm commented 2 years ago

No, we shouldn't conflicts with them. They should provides us, and also conflicts with us.

ISSOtm commented 2 years ago

Do we really conflict with skia-git? If so, that should be reinstated in the conflicts array

I can't imagine why we'd conflict with skia-git, probably shouldn't re-add it to conflicts.

I was thinking there could be a lib search path conflict, but I really don't think it can happen, so this is very likely the right move imo.

ISSOtm commented 2 years ago

The last thing I want to check is compiling less things into Skia, to reduce build time. My plan is to remove libs and see if compilation fails. AIUI, since we are statically linking to Skia, if a function relied on by Aseprite is not present, we should get a build-time error. (Right?)

ISSOtm commented 2 years ago

Bruh, I'm stupid.

ISSOtm commented 2 years ago

Sorry about that, I added the missing file. My bad!

ImperatorStorm commented 2 years ago

Run tests? E.g. LAF's, except that they fail to build due to an obscure linker error (looks like some static lib requires a dynamic lib that doesn't get -l'd?)

Should probs give it a try.

wait on this or merge?

ISSOtm commented 2 years ago

Good enough imo. Let's not wait indefinitely on the changes.

ImperatorStorm commented 2 years ago

Manually merged into master.

Thanks for rewriting the package!

ISSOtm commented 2 years ago

Thank you for collaborating as well! :)

ImperatorStorm commented 2 years ago

squash+merged into aseprite because AUR got mad when i tried to push aseprite.