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

oonf_api requires undocumented define #2772

Closed
OlegHahm opened this issue Apr 9, 2015 · 7 comments · Fixed by #2805
Closed

oonf_api requires undocumented define #2772

OlegHahm opened this issue Apr 9, 2015 · 7 comments · Fixed by #2805
Assignees
Labels
Area: network Area: Networking Area: pkg Area: External package ports Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Comments

@OlegHahm
Copy link
Member

OlegHahm commented Apr 9, 2015

Building an application (e.g. https://github.com/Lotterleben/RIOT-AODVv2/tree/master/aodvv2_demo) with oonf_api fails with an error like

In file included from /home/oleg/git/RIOT/pkg/oonf_api/oonf_api/src-api/common/avl_comp.h:46:0,
                 from /home/oleg/git/RIOT/pkg/oonf_api/oonf_api/src-api/common/avl_comp.c:45:
/home/oleg/git/RIOT/pkg/oonf_api/oonf_api/src-api/common/netaddr.h: In function ‘netaddr_socket_get_addressfamily’
/home/oleg/git/RIOT/pkg/oonf_api/oonf_api/src-api/common/netaddr.h:348:16: error: ‘sockaddr6_t’ has no member named ‘__in6_a’
   return s->std.sin6_family;

if one does not define the macro RIOT (which is done by the given example in the application Makefile by
export CFLAGS += -DRIOT

I think this define is useless, because it's only used in a RIOT specific patch (https://github.com/RIOT-OS/RIOT/blob/master/pkg/oonf_api/0001-add-RIOT-support.patch#L87) that can live without this ifdeff since the package inside RIOT will always be built for RIOT.

If there's still a reason for this define, it should be either documented somewhere and get a better name.

@OlegHahm OlegHahm added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking Area: pkg Area: External package ports labels Apr 9, 2015
@OlegHahm
Copy link
Member Author

OlegHahm commented Apr 9, 2015

Thanks to @mfrey for detecting this.

@Lotterleben
Copy link
Member

Seems reasonable to me. @benpicco, since you're the author of this patch, can you tell us if you had any other intentions we're overlooking here?

@benpicco
Copy link
Contributor

I think the intention was to have this always defined by a core RIOT Makefile, but that never got merged. I wanted to compile the same source code for RIOT and for Linux, so I needed a way to identify the platform - maybe something like this has been added by now? Otherwise it can't hurt to add such define to Makefile.include, better name it __RIOT__ for consistency.

@OlegHahm
Copy link
Member Author

I'm not sure if we need this. Is it really a use case to build a thirdparty library inside RIOT (i.e. in $(RIOTBASE)/pkg/) for Linux?

@benpicco
Copy link
Contributor

Well it's possible that one day such patches could be merged upstream, then they mustn't break the build for non-RIOT targets.

@OlegHahm
Copy link
Member Author

Ok, makes sense. Let's close this as soon as #2805 is merged.

@Lotterleben
Copy link
Member

Will keep an eye on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: pkg Area: External package ports Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants