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

libpaper: remove _NL_PAPER_{WIDTH,HEIGHT} #21510

Merged
merged 2 commits into from
Oct 22, 2024
Merged

Conversation

knyipab
Copy link
Contributor

@knyipab knyipab commented Sep 19, 2024

Discovered that _NL_PAPER_WIDTH and _NL_PAPER_HEIGHT are not the size but they are C++ enum items (e.g. defined in langinfo.h) that is the input of std c lib func nl_langinfo() . Feeding 210 and 297 into that func may have unknown behavior.

I discovered this as I am trying to build libreoffice and I encountered the similar issue in their paper.cxx.

It may not have material impact on existing package, but just for correctness.

@knyipab knyipab marked this pull request as ready for review September 19, 2024 14:11
@Biswa96
Copy link
Member

Biswa96 commented Sep 19, 2024

Discovered that _NL_PAPER_WIDTH and _NL_PAPER_HEIGHT are not the size but they are C++ enum items

That explains the removal of CFLAGS+=" -D_NL_PAPER_WIDTH=210 -D_NL_PAPER_HEIGHT=297" 👍

Could you explain why the patch file was added?

@knyipab
Copy link
Contributor Author

knyipab commented Sep 19, 2024

Cuz android's bionic (langinfo.h) never define or provide _NL_PAPER_WIDTH and _NL_PAPER_HEIGHT at all, so there is no such thing nl_langinfo(_NL_PAPER_WIDTH). Libreoffice's paper.cxx basically skip that part of locale paper size detection with && !defined(ANDROID). See:
https://github.com/LibreOffice/core/blob/7db1150b2c4a9adb993084d2dceedb450781b1dc/i18nutil/source/utility/paper.cxx#L308-L317

Note; libreoffice's ANDROID is probably configured by their makefile and does not work, so I patch that to !defined(__ANDROID__)

@Biswa96
Copy link
Member

Biswa96 commented Sep 19, 2024

Thanks for the explanation. I think this change can be added in upstream.

__ANDROID__ is the correct one https://android.googlesource.com/platform/bionic/+/HEAD/docs/defines.md

@knyipab
Copy link
Contributor Author

knyipab commented Sep 20, 2024

Okay, made one here: rrthomas/libpaper#59
Last commit is six months ago and will see if they will merge and make a new release.

@landfillbaby
Copy link
Member

landfillbaby commented Sep 20, 2024

alternatively, partially revert 2148a3e, to this:

termux_step_pre_configure() {
	# 210x297 is A4 size. Hard code as default.
	sed -i \
		-e "s|NL_PAPER_GET(_NL_PAPER_WIDTH)|210|g" \
		-e "s|NL_PAPER_GET(_NL_PAPER_HEIGHT)|297|g" \
		"${TERMUX_PKG_SRCDIR}"/lib/libpaper.c.in.in
}

or write an equivalent patch

@knyipab
Copy link
Contributor Author

knyipab commented Sep 21, 2024

I do not hold strong view nor any preference on which approach. I just want to flag it. Feel free to change this PR.

@twaik
Copy link
Member

twaik commented Oct 19, 2024

Tuxpaint works fine, no errors.

@twaik
Copy link
Member

twaik commented Oct 19, 2024

I'll push these changes in 48 hours if no one minds.
Or someone else can do that earlier.

@knyipab
Copy link
Contributor Author

knyipab commented Oct 19, 2024

I am open to any decisions (merge or not).

Just FYI, the author has updated the master branch to handle this issue and ask for test. But I did not figure out a way to build the it from the git branch which needs to run ./bootstrap. Therefore, I did not get back to confirm the author for quite a while.

@twaik twaik merged commit d3c5e37 into termux:master Oct 22, 2024
7 checks passed
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.

5 participants