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

Re mk cross build2 #1161

Merged
merged 4 commits into from
Jan 5, 2021
Merged

Re mk cross build2 #1161

merged 4 commits into from
Jan 5, 2021

Conversation

cspiel1
Copy link
Collaborator

@cspiel1 cspiel1 commented Dec 23, 2020

This fixes cross compilation builds without having to set both SYSROOT and
SYSROOT_LOCAL.

More concretely, it avoids that the fallback value of SYSROOT_LOCAL leads to:

 cc1: warning: include location "/usr/local/include/rem" is unsafe for cross-compilation [-Wpoison-system-directories]

Also LIBREM_PATH was made to always point to the rem directory and added LIBREM_INC and LIBREM_SO.

Not part of this PR:
Another improvement would be to add a rem.mk to rem that adequately sets LIBREM_PATH, LIBREM_INC and LIBREM_SO similar to re.mk for the LIBRE_xxx variables. Let me know if you prefer this!

@cspiel1 cspiel1 changed the title Makefile: set SYSROOT_LOCAL only if SYSROOT is the default /usr Re mk cross build2 Dec 23, 2020
@cspiel1 cspiel1 marked this pull request as draft December 23, 2020 12:42
@cspiel1 cspiel1 force-pushed the re_mk_cross_build2 branch 6 times, most recently from 3fc9a31 to 505ac87 Compare December 23, 2020 13:51
@cspiel1 cspiel1 marked this pull request as ready for review December 23, 2020 13:56
@cspiel1 cspiel1 requested a review from sreimers December 23, 2020 14:02
make V=1 CCACHE= info test modules
make clean; make CCACHE= STATIC=yes
make V=1 CCACHE= SYSROOT_LOCAL=/usr/local info info2 test modules
make clean; make CCACHE= SYSROOT_LOCAL=/usr/local STATIC=yes
Copy link
Member

Choose a reason for hiding this comment

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

macOS should not require any special args to build. A simple make should be sufficient to build baresip on all supported platforms.

Copy link
Collaborator Author

@cspiel1 cspiel1 Dec 23, 2020

Choose a reason for hiding this comment

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

But if one specifies SYSROOT for a cross compilation build then it is not common to include /usr/local into the header search paths.
e.g. yocto says:

cc1: warning: include location "/usr/local/include/rem" is unsafe for cross-compilation [-Wpoison-system-directories]

I couldn't see where SYSROOT is set for the maxOS build. Can we fix this differently?

Copy link
Member

@sreimers sreimers Dec 23, 2020

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now I added /usr/local as fallback. Also no warning/error in our yocto build.

@@ -79,5 +79,5 @@ jobs:
- name: make baresip macOS
if: ${{ runner.os == 'macOS' }}
run: |
make V=1 CCACHE= info test modules
make clean; make CCACHE= STATIC=yes
make V=1 CCACHE= SYSROOT_LOCAL=/usr/local info info2 test modules
Copy link
Member

Choose a reason for hiding this comment

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

is info2 only for debugging used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Like info. The info target is defined in re.mk. And the variables LIBREM_PATH, ... are not included. Should I remove it or rename it?

@cspiel1 cspiel1 marked this pull request as draft December 23, 2020 17:12
@cspiel1 cspiel1 force-pushed the re_mk_cross_build2 branch 4 times, most recently from a415fef to 77a6919 Compare December 23, 2020 18:14
@cspiel1 cspiel1 marked this pull request as ready for review December 23, 2020 18:20
Makefile Outdated
@@ -329,3 +367,10 @@ src/static.c: $(BUILD) Makefile $(APP_MK) $(MOD_MK)
done
@echo " NULL" >> $@
@echo "};" >> $@

.PHONY: info2
info2:
Copy link
Member

Choose a reason for hiding this comment

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

@alfredh
Copy link
Collaborator

alfredh commented Dec 29, 2020

I tested on OSX with GNU make 3.81 and got this error:

Makefile:371: *** target file `info' has both : and :: entries.  Stop.

@sreimers
Copy link
Member

Makefile:371: *** target file `info' has both : and :: entries.  Stop.

Looks like this re commit baresip/re@432fa6a is missing?

@alfredh
Copy link
Collaborator

alfredh commented Dec 29, 2020

If you want to use double-colon targets, it makes a hard depencency that baresip and re
must use an exact version of the makefile or later.

baresip (commit 29. dec) -> re (commit 29. dec)

it will for example make it difficult to use baresip from 29. dec +/- 1 day with libre 28. dec or older.

it is guaranteed that this will cause many support requests from users.

a simpler and more flexible solution is:

bareinfo: info
	@echo "  LIBREM_PATH:   $(LIBREM_PATH)"
	@echo "  LIBREM_INC:    $(LIBREM_INC)"
	@echo "  LIBREM_SO:     $(LIBREM_SO)"

This fixes cross compilation builds without having to set both SYSROOT and
SYSROOT_LOCAL.
We always set an adequate LIBREM_PATH and add new variables LIBREM_INC,
LIBREM_SO that are used for CFLAGS and LFLAGS.

Check if ../rem directory exists first. Then check if rem.h exists in
SYSROOT_LOCAL first. At least in SYSROOT.
Similar to LIBRE_INC and LIBRE_SO in re.mk.
@cspiel1 cspiel1 force-pushed the re_mk_cross_build2 branch from ecbe1fa to 3b4031d Compare January 4, 2021 13:02
@cspiel1
Copy link
Collaborator Author

cspiel1 commented Jan 5, 2021

From my side it is ready to merge now. Tested in yocto recipe:

export LIBRE_MK   = "${STAGING_DIR_TARGET}/usr/share/re/re.mk"
export SYSROOT    = "${STAGING_DIR_TARGET}/usr"
export DESTDIR    = "${D}"

@alfredh alfredh merged commit 9bee0b1 into baresip:master Jan 5, 2021
@alfredh
Copy link
Collaborator

alfredh commented Jan 5, 2021

thanks!

@cspiel1 cspiel1 deleted the re_mk_cross_build2 branch January 5, 2021 14:21
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