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.

Sign up for GitHub

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

Jump to bottom

configure: Use AC_C_BIGENDIAN macro to check endianness. #2477

Closed
wants to merge 2 commits into from
Closed

configure: Use AC_C_BIGENDIAN macro to check endianness. #2477

wants to merge 2 commits into from

Conversation

tsutsui
Copy link

@tsutsui tsutsui commented Feb 22, 2023

__BYTE_ORDER is Linux specific (NetBSD has _BYTE_ORDER) but AC_C_BIGENDIAN seems enough.
https://www.gnu.org/software/autoconf/manual/autoconf-2.71/html_node/C-Compiler.html#index-AC_005fC_005fBIGENDIAN-1

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Feb 25, 2023
@tsutsui
ibus: update to 1.5.28.
90c8c79 
pkgsrc changes:
- explicitly use updated unicode-character-database 15.0.0 and
  unicode-emoji 15.0
- remove -ldl workaround since pull request has been applied in upstream
 ibus/ibus#2442
- fix endianness check in configure
 ibus/ibus#2477
- workaround X11 locale Compose directory problem (${PREFIX}/lib/X11/locale
  vs ${PREFIX}/share/X11/locale) on NetBSD native X11
 ibus/ibus#2478

Upstream changes:
 https://github.com/ibus/ibus/releases/tag/1.5.28

1.5.28

  * Implement new process_key_event for ibus-x11 506ac99
  * Convert internal EN compose table to GResource 19ca106 ae69635 bf8848e
  * Enhance surrounding text 8d0abbc 781119b ddead51 2a235c8
  * Enhance CI 2719e93 2555fa9 a3a9bd3 f3a7772
  * Update ibusunicodegen.h with Unicode 15.0.0 8f00d67
  * Update simple.xml with xkeyboard-config 2.38-1 594ec48 3c51582
  * Fix SEGVs cd621f8 ecfae16
  * Release 1.5.28 bc065f8

Code Contributors:

  * tools: Check libdl for dlclose() properly in configure (Izumi Tsutsui)
    babad78
  * configure: Fix texts for surrounding text (Eberhard Beilharz) d190bc3
  * Add active-surrounding-text property to IBusEngine (Philippe Rouquier)
    bd24be4

Translation Contributors:

  * po: Update translation (Georgian) (Temuri Doghonadze) 382c034
  * po: Update translation (Spanish) (Emilio Herrera) 8abf3eb
  * po: Update translation (Turkish) (Oguz Ersen) f3a9983
  * po: Update translation (Russian) (Alexey Rubtsov) 4b0e4c5
  * po: Update translation (Korean) (simmon) 41e33c2
  * po: Update translation (French) (Julien Humbert) 2f897b5
  * po: Update translation (Italian) (Luis Pixeltux) f6eabce
@tsutsui tsutsui mentioned this pull request Mar 5, 2023
Open
@tsutsui
Copy link
Author

tsutsui commented Mar 9, 2023

I notice there are extra AC_MSG_CHECKING and AC_MSG_RESULT messages so removed them in f733c18.

--- configure.log.orig	2023-03-10 01:11:14.100489079 +0900
+++ configure.log	2023-03-10 01:10:19.408079100 +0900
@@ -207,8 +207,7 @@
 checking for sys/prctl.h... no
 checking for daemon... yes
 checking for dlclose in -lc... yes
-checking build system endianness... checking whether byte ordering is bigendian... no
-little
+checking whether byte ordering is bigendian... no
 checking for pkg-config... /usr/pkg/bin/pkg-config
 checking pkg-config is at least version 0.16... yes
 checking for GLIB... yes

fujiwarat
fujiwarat requested changes Mar 23, 2023
View reviewed changes
Copy link
Member

@fujiwarat fujiwarat left a comment

Choose a reason for hiding this comment

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

Please delete the last period in the commit subject.
Also please append BUG=https://github.com/ibus/ibus/pull/2477 in the commit description.

configure.ac
)],
[ENDIAN=big]
)
AC_C_BIGENDIAN([ENDIAN=big],[ENDIAN=little],[ENDIAN=unknown],[])
Copy link
Member

Choose a reason for hiding this comment

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

Please add a space char by comma char.
Probably I think "[ENDIAN=big]" can be applied to the 4th argument for the universal case at the moment.

configure.ac
fi
AC_MSG_RESULT($ENDIAN)
AC_SUBST(ENDIAN)
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 to append ENDIAN="$ENDIAN ($ac_cv_c_bigendian, use ac_cv_c_bigendian to yes or no)" next to AC_SUBST(ENDIAN) .

Copy link
Author

Choose a reason for hiding this comment

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

I think to append ENDIAN="$ENDIAN ($ac_cv_c_bigendian, use ac_cv_c_bigendian to yes or no)" next to AC_SUBST(ENDIAN) .

Sorry, I don't understand what the statement you suggested means.

Copy link
Member

Choose a reason for hiding this comment

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

The change will effect the configure result. E.g.
https://github.com/ibus/ibus/actions/runs/4376470045/jobs/7895795597?pr=2477#step:7:385

Copy link
Author

Choose a reason for hiding this comment

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

fi
-AC_MSG_RESULT($ENDIAN)
AC_SUBST(ENDIAN)

The change will effect the configure result. E.g.
https://github.com/ibus/ibus/actions/runs/4376470045/jobs/7895795597?pr=2477#step:7:385

Sorry I committed multiple revisions and github UI doesn't quote commentted lines exactly.
Which reivision/line is 'the change' you mean?

If you meant AC_MSG_RESULT($ENDIAN) line removed in f733c18 :
AC_MSG_RESULT() just print a result of a check and won't affect a value itself.
https://www.gnu.org/software/autoconf/manual/autoconf-2.71/html_node/Printing-Messages.html

If you meant 'the change' is to set/update ENDIAN value by
ENDIAN="$ENDIAN ($ac_cv_c_bigendian, use ac_cv_c_bigendian to yes or no)" next to AC_SUBST(ENDIAN)
line to print endianness description in AC_MSG_RESULT in configure.ac

ibus/configure.ac

Line 905 in a4007f7

Build endianness $ENDIAN

an actual value of $ENDIAN is already set by
AC_C_BIGENDIAN([ENDIAN=big],[ENDIAN=little],[ENDIAN=unknown],[])
line and this new one will be overwritten the value.
I wonder what happens on reference of $ENDIAN in and it's also referred in src/Makefile.am

ibus/src/Makefile.am

Line 271 in a4007f7

compose/sequences-$(ENDIAN)-endian: gen-internal-compose-table

Copy link
Member

Choose a reason for hiding this comment

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

an actual value of $ENDIAN is already set by AC_C_BIGENDIAN([ENDIAN=big],[ENDIAN=little],[ENDIAN=unknown],[]) line and this new one will be overwritten the value.

That's why I asked to update ENDIAN after AC_SUBST(ENDIAN)

 AC_SUBST(ENDIAN)
+ENDIAN="$ENDIAN ($ac_cv_c_bigendian, use ac_cv_c_bigendian to yes or no)"

Copy link
Member

@fujiwarat fujiwarat Mar 27, 2023 •
edited

Choose a reason for hiding this comment

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

Sorry, even if ENDIAN is updated after AC_SUBST(ENDIAN), the overriding ENDIAN reflects the Makefile recursively.

ENDIAN_MSG="$ENDIAN ($ac_cv_c_bigendian, use ac_cv_c_bigendian to yes or no)"

and

-  Build endianness              $ENDIAN
+  Build endianness              $ENDIAN_MSG

Copy link
Member

Choose a reason for hiding this comment

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

Are you fine to update the configure result?

Copy link
Author

Choose a reason for hiding this comment

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

Are you fine to update the configure result?

No problem for me.

@tsutsui
Copy link
Author

tsutsui commented Mar 25, 2023

Please delete the last period in the commit subject. Also please append BUG=https://github.com/ibus/ibus/pull/2477 in the commit description.

Well there is no way to predict a pull request number before commit.
Do you mean I should update the first commit and push my branch with git push -f origin ac_c_bigendian ?

@fujiwarat
Copy link
Member

Well there is no way to predict a pull request number before commit. Do you mean I should update the first commit and push my branch with git push -f origin ac_c_bigendian ?

Right, it helps me. I also will modify your patch if you're fine.

@tsutsui
Copy link
Author

tsutsui commented Mar 26, 2023

Right, it helps me. I also will modify your patch if you're fine.

I can update commits to improve implementation details, but it's really hard to reflect design changes without written documents.
I don't mind you modify my patch (because my main subject is CI on NetBSD).

fujiwarat added a commit that referenced this pull request Apr 5, 2023
@fujiwarat
configure: Use AC_C_BIGENDIAN for BSD systems
630f3f7 
NetBSD does not define __BYTE_ORDER and use AC_C_BIGENDIAN instead.

BUG=#2477
@fujiwarat fujiwarat added this to the 1.5.29 milestone Apr 5, 2023
@fujiwarat fujiwarat closed this Apr 5, 2023
@tsutsui tsutsui deleted the ac_c_bigendian branch August 8, 2023 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujiwarat fujiwarat fujiwarat requested changes

Assignees
No one assigned
Labels
ci ibus Type-Enhancement UNIX-BSD
Projects
None yet
Milestone
1.5.29
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tsutsui @fujiwarat