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

A few minor buildsystem tweaks #817

Merged
merged 6 commits into from
Feb 18, 2019
Merged

A few minor buildsystem tweaks #817

merged 6 commits into from
Feb 18, 2019

Conversation

NiLuJe
Copy link
Member

@NiLuJe NiLuJe commented Feb 16, 2019

  • Avoid compat CFLAGS on kobo & kindle-legacy when using our TCs
  • Disable ARM ASM in MµPDF, and honor debug builds

I somehow managed to miss kobo & kindle-legacy targets the last time we went over this...
It's potentially crashy, and impossible to debug.
c.f., https://bugs.ghostscript.com/show_bug.cgi?id=698879
      koreader#816

Also, honor debug builds in MuPDF (as it enforces CFLAGS).
@NiLuJe
Copy link
Member Author

NiLuJe commented Feb 16, 2019

AFAICT, it's really ARM asm, not NEON.
I couldn't really feel any performance impact in what little I tested, and that was in a debug build.

@@ -71,6 +71,8 @@ set(PATCH_CMD3 sh -c "patch -N -p1 < ${CMAKE_CURRENT_SOURCE_DIR}/libjpeg_shared.
# NOTE: There's also a few ARCH_UNALIGNED_OK checks, but we never did pass that define.
# (FWIW, we *could*, on anything not kindle-legacy).
set(PATCH_CMD4 sh -c "patch -N -p1 < ${CMAKE_CURRENT_SOURCE_DIR}/no_arm_asm.patch || true")
# Honor CFLAGS
set(PATCH_CMD5 sh -c "patch -N -p1 < ${CMAKE_CURRENT_SOURCE_DIR}/honor_cflags.patch || true")
Copy link
Member

Choose a reason for hiding this comment

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

Getting pretty complex there, lol.

else()
set(MUPDF_BUILD_TYPE "release")
endif()
set(BUILD_CMD_GENERATE sh -c "env CFLAGS=\"${HOSTCFLAGS}\" $(MAKE) -j${PARALLEL_JOBS} generate build=\"${MUPDF_BUILD_TYPE}\" CC=\"${HOSTCC}\" verbose=\"yes\"")
Copy link
Member

Choose a reason for hiding this comment

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

Is 1 and yes equivalent here or was the 1 not doing anything?

Copy link
Member Author

@NiLuJe NiLuJe Feb 16, 2019

Choose a reason for hiding this comment

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

The Makefile is explicitly doing ifneq "$(verbose)" "yes", so, yeah, if the intent was to get verbose builds, that wasn't doing the trick ;).

Whether we actually want a verbose build here is debatable, though ;).
You know my position on the subject, but I'm perfectly okay with switching that back to "no" here ;).

This comment was marked as outdated.

Copy link
Member

Choose a reason for hiding this comment

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

Verbose all the way as far as I'm concerned. It's just that too much of it can cause trouble with one of the CI services (definitely Travis and GitLab, possibly CircleCI as well).

Copy link
Member

Choose a reason for hiding this comment

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

I think at least one of them (not Travis, certainly not GitLab) might do something more akin to a terminal emulator (i.e., just a limited amount of scrollback), which sounds potentially a lot more useful to catch the error.

Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

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

Anyway, lgtm, although I don't really know how to gauge the impact of that ARM ASM patch.

Buildsystem appears to behave with the CFLAGS tweaks.
Use "no" to make it clearer that, if we wanted verbose builds, we do
have to explicitly use "yes" ;).
@NiLuJe
Copy link
Member Author

NiLuJe commented Feb 16, 2019

From what I've seen, it's literally nearly a decade old ARM inline asm (switching from thumb to ARM mode and everything!).

I'm fairly confident GCC should nowadays be doing something fairly non-stupid with the pure C versions.

@NiLuJe NiLuJe merged commit abc24ac into koreader:master Feb 18, 2019
@NiLuJe
Copy link
Member Author

NiLuJe commented Feb 18, 2019

(I'll bump base tomorrow, already too much new stuff in the next nightly ;)).

NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Feb 18, 2019
Picks up:
* PB fb setup cleanups koreader/koreader-base#813
* Buildsystem tweaks koreader/koreader-base#817
NiLuJe added a commit to koreader/koreader that referenced this pull request Feb 18, 2019
* Bump base
  * PB fb setup cleanups (koreader/koreader-base#813)
  * Buildsystem tweaks (koreader/koreader-base#817)
  * Disable HW dithering on Kindle koreader/koreader-base#819)
mwoz123 pushed a commit to mwoz123/koreader that referenced this pull request Mar 29, 2020
* Bump base
  * PB fb setup cleanups (koreader/koreader-base#813)
  * Buildsystem tweaks (koreader/koreader-base#817)
  * Disable HW dithering on Kindle koreader/koreader-base#819)
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.

2 participants