Add USEPKG variable for Makefiles #576

Kijewski · 2014-01-28T17:06:08Z

Currently pkg/USING says one should use EXTERNAL+=$(RIOTBASE)/pkg/<pkg_name> to enable PKG modules. Using this line the PKG will be compiled but not linked.

This change adds a USEPKG variable to be used like USEPKG += <pkg_name>, which looks less clumsy and gets the PKG linked in the binary.

LudwigKnuepfer · 2014-01-28T17:29:01Z

That's two changes:
One is the rename and redefinition of the variable.
The other is adding the resulting archive to baselibs.
Right?

I am all in favor of the first.
Did you check whether the second change works for the existing packages?

Kijewski · 2014-01-28T17:35:47Z

You could not find any use of EXTERNAL ("git grep EXTERNAL").

In Makefile.include it is called EXTERNAL_MODULES, but pkg/USING calls it EXTERNAL.

LudwigKnuepfer · 2014-01-28T17:44:33Z

I'm checking with https://github.com/authmillenon/projects/tree/libcoap

LudwigKnuepfer · 2014-01-28T17:49:05Z

Appears to work.

Kijewski · 2014-01-28T18:17:52Z

Hm, maybe the PR should be extended:

Now you have to do export INCLUDES += -I$(RIOTBASE)/pkg/libcoap/libcoap yourself.
It would be possible to do INCLUDES += -I$(RIOTBASE)/pkg/$(PKG_NAME)/include for every PKG_NAME in USEPKG automatically.
The two existing PKGs would have to be changed.
The PKGs would have to be built before the board and project files (b/c the include files might have to be checked out).

I guess that would make to even easier to use packages. Should I amend my PR, or wait to open a new one? (Or don't you like my idea?)

LudwigKnuepfer · 2014-01-28T18:26:22Z

I'm not sure INCLUDES += -I$(RIOTBASE)/pkg/$(PKG_NAME)/include is a valid assumption...

LudwigKnuepfer · 2014-01-28T18:30:42Z

Of course we could make it a requirement. That would only require a documentation update ;-)

I like the idea in any case.

Maybe someone else could add some brain power to this.. @authmillenon @thomaseichinger ?

Kijewski · 2014-01-28T20:36:08Z

(Rebased.) My second commit implements automatic adding to INCLUDES.
The PKGs are built before any other .o file, so there won't be any chicken-and-egg problem. (Compiling fails b/c include files are missing, include files won't get downloaded b/c of earlier compiling errors.)

I added the include folder for libcoap, but openwsn had multiple patching errors for me.

LudwigKnuepfer · 2014-01-29T09:23:23Z

openwsn appears to build (and patch) for RIOT/master with https://github.com/thomaseichinger/projects/tree/openwsn-pkg

thomaseichinger · 2014-01-29T11:34:31Z

ACK from my side regarding the USEPKG variable.
It surly has a clearer usage than one has to state EXTERNAL_MODULE and USEMODULE and define PROJDEPS for includes or something.

@Kijewski could you provide me a build log and the revision/branch/etc. these patching errors occur on so I can investigate them?

miri64 · 2014-01-30T17:50:38Z

I would rather let the package maintainers let configure the include path if possible (because situations might not be as easily to solve as in libcoap). Also: if we decide against this could you at least make this change a patch for the libcoap package and not a static change in RIOT (maybe libcoap gets a include directory in the future to). But for the rest I'm totally for it.

miri64 · 2014-01-30T17:58:39Z

Furthermore, the original EXTERNAL_MODULE definition was originally from @benpicco and I am not sure about his use-case for this. Can we get some feedback regarding this @benpicco?

benpicco · 2014-01-31T03:15:03Z

Well EXTERNAL_MODULE was to include modules that are not part of the RIOT source tree, but I'm converting my use cases to PKG - I don't mind it being dropped.

OlegHahm · 2014-02-05T18:27:02Z

I go with @authmillenon. ACK for everything except the include path change.

Kijewski · 2014-02-05T18:41:29Z

I still think that the include path change is a good idea. To quote @authmillenon: "situations might not be as easily to solve as in libcoap". That's the reason why a standardized include folder would be useful. Otherwise you still have to define two variables in your project (USEPKG and INCLUDES).

If there are deep changes to the include structure, then the forward compatibility of the package will be broken anyways.

But maybe a Makefile.include for the PKGs would be a better option? With -include make won't cry if /pkg/$(PKG)/Makefile.include is missing.

benpicco · 2014-02-05T18:42:09Z

Ok, now that I've looked closer into it I'm confused, libcoap is actually using EXTERNAL_MODULE + USEMODULE [1] andEXTERNAL` from the pkg documentation is indeed never called anywhere, so it might as well be a typo.

[1] https://github.com/authmillenon/projects/blob/libcoap/libcoap/Makefile#L37

OlegHahm · 2014-02-05T18:44:28Z

@Kijewski, +1 for adding a Makefile.include to the PKG.

benpicco · 2014-02-05T18:50:40Z

How would a standardized include directory work without major changes to the upstream project? e.g. oonf_api has includes in src-api/common/ and src-api/rfc5444 if those two modules are used. With the current EXTERNAL_MODULE + USEMODULE, it's also possible for packages to offer multiple modules.

Kijewski · 2014-02-05T19:24:53Z

@benpicco it never occurred to me that one would want to have multiple modules in one package. Is this really a use case?

benpicco · 2014-02-05T19:29:10Z

Well it's not absolutely necessary, it could as well be compiled into one monolithic module, but header files are still distributed over several subfolders.

LudwigKnuepfer · 2014-02-06T08:24:20Z

@benpicco You could just ln -s all includes into one include directory.

benpicco · 2014-02-07T00:35:10Z

You could just ln -s all includes into one include directory.

That doesn't sound like a good idea, I see this breaking in … interesting ways…

Also, do I understand correctly that this pull request only renames EXTERNAL_MODULE (it's wrong in the documentation) to USEPKG and removes the need to call USEMODULE?

I don't see a huge benefit in the latter, it's consistent with the rest of RIOT.

LudwigKnuepfer · 2014-02-07T08:28:51Z

Can you elaborate on the interesting ways?

benpicco · 2014-02-07T09:12:42Z

In case of oonf_api, it's common for header files to include types defined in other header files like

#include "common/common_types.h"
#include "rfc5444/rfc5444_api_config.h"

Aside from that, links will also get out of sync when files are renamed/deleted.

LudwigKnuepfer · 2014-02-07T09:27:51Z

You're right.

Kijewski · 2014-02-07T18:03:26Z

I don't think that changing file names is a problem. A PKG should check out a tag.

Using ln -s is not the best option is this case, because of the relative includes.
But I see no shortcomings at all if you do something like:

for all files $f in [list of files]:
     echo '#include "../some/path/$f.h"' > ./include/$f.h

Kijewski · 2014-02-10T22:30:37Z

Ping.

LudwigKnuepfer · 2014-02-10T23:49:11Z

@benpicco the ping was for you I assume.

benpicco · 2014-02-11T00:23:41Z

I find that worse than specifying the includes manually as it currently is, I'd rather +1 the Makefile.include for pkg

ifneq (,$(filter some_module,$(USEMODULE)))
    INCLUDES += $(CURDIR)/some_module/path/to/include
endif
…
export INCLUDES

(or USEPKG if you want to have each PKG be a module)

Kijewski · 2014-02-11T03:55:42Z

My last patch does that: PKGs will have to define a Makefile.include target. In the easiest case this is just

Makefile.include:
        @true

with a static pkg/%/Makefile.include.

pkg/%/Makefile.include will be loaded after the "mandatory includes".

What pkg/%/Makefile.include should contain is up to the PKG. The easiest (and hopefully most common) cases are

INCLUDES += -I $(RIOTBASE)/pkg/.../include

or even an empty file.

Kijewski · 2014-02-12T12:15:27Z

Are there comments on my proposed interface, @benpicco, @LudwigOrtmann, @authmillenon?
I think it's very easy to implement for a PKG developers, and even easier to use in applications.

miri64 · 2014-02-12T12:31:55Z

With the following change and a rebase you'll get my ACK ;-)

diff --git a/pkg/libcoap/Makefile.include b/pkg/libcoap/Makefile.include
index 0fbf281..e69de29 100644
--- a/pkg/libcoap/Makefile.include
+++ b/pkg/libcoap/Makefile.include
@@ -1 +0,0 @@
-INCLUDES += -I $(RIOTBASE)/pkg/libcoap/include
+INCLUDES += -I $(RIOTBASE)/pkg/libcoap/libcoap
diff --git a/pkg/libcoap/include/coap.h b/pkg/libcoap/include/coap.h
deleted file mode 100644
index b61cbd7..0000000
--- a/pkg/libcoap/include/coap.h
+++ /dev/null
@@ -1 +0,0 @@
-#include "../libcoap/coap.h"

Kijewski · 2014-02-12T12:41:46Z

An empty Makefile.include for libcoap? Shouldn't that be

-INCLUDES += -I $(RIOTBASE)/pkg/libcoap/include
+INCLUDES += -I $(RIOTBASE)/pkg/libcoap/libcoap

miri64 · 2014-02-12T12:48:48Z

I've realized that myself. That's why I edited the comment ;)

Currently pkg/USING says one should use `EXTERNAL+=$(RIOTBASE)/pkg/<pkg_name>` to enable PKG modules. Using this line the PKG will be compiled but not linked. This change adds a USEPKG variable to be used like `USEPKG += <pkg_name>`, which looks less clumsy and gets the PKG linked in the binary.

Packages have to define a Makefile.include target in pkg/%/Makefile. pkg/%/Makefile.include will be loaded after all other dependencies.

Kijewski · 2014-02-12T13:02:44Z

Rebased.

miri64 · 2014-02-12T13:23:50Z

ACK

benpicco · 2014-02-12T17:32:43Z

Looks good, I'll update #630 as well

Kijewski · 2014-02-13T23:37:35Z

Does someone want to hit the merge button? :)

LudwigKnuepfer · 2014-02-13T23:49:11Z

Feel free to do it yourself if you are confident, it's your PR after all ;-)

Add USEPKG variable for Makefiles

ghost assigned miri64 Jan 28, 2014

LudwigKnuepfer mentioned this pull request Feb 6, 2014

[RFC] add oonf_api to pkg #630

Merged

Kijewski added 3 commits February 12, 2014 13:57

Use include path convention for PKGs

84cffaa

Packages have to define a Makefile.include target in pkg/%/Makefile. pkg/%/Makefile.include will be loaded after all other dependencies.

Apply @authmillenon's patch

ee6f683

Kijewski added a commit that referenced this pull request Feb 14, 2014

Merge pull request #576 from Kijewski/usepkg

4b012df

Add USEPKG variable for Makefiles

Kijewski merged commit 4b012df into RIOT-OS:master Feb 14, 2014

Kijewski deleted the usepkg branch February 14, 2014 13:40

miri64 mentioned this pull request Feb 19, 2014

pkg : libcoap : Update libcoap version #740

Merged

jcarrano mentioned this pull request Nov 7, 2018

Makefile.include: remove rule to remake PKGs' Makefile.include #10344

Merged

Add USEPKG variable for Makefiles #576

Add USEPKG variable for Makefiles #576

Uh oh!

Conversation

Kijewski commented Jan 28, 2014

Uh oh!

LudwigKnuepfer commented Jan 28, 2014

Uh oh!

Kijewski commented Jan 28, 2014

Uh oh!

LudwigKnuepfer commented Jan 28, 2014

Uh oh!

LudwigKnuepfer commented Jan 28, 2014

Uh oh!

Kijewski commented Jan 28, 2014

Uh oh!

LudwigKnuepfer commented Jan 28, 2014

Uh oh!

LudwigKnuepfer commented Jan 28, 2014

Uh oh!

Kijewski commented Jan 28, 2014

Uh oh!

LudwigKnuepfer commented Jan 29, 2014

Uh oh!

thomaseichinger commented Jan 29, 2014

Uh oh!

miri64 commented Jan 30, 2014

Uh oh!

miri64 commented Jan 30, 2014

Uh oh!

benpicco commented Jan 31, 2014

Uh oh!

OlegHahm commented Feb 5, 2014

Uh oh!

Kijewski commented Feb 5, 2014

Uh oh!

benpicco commented Feb 5, 2014

Uh oh!

OlegHahm commented Feb 5, 2014

Uh oh!

benpicco commented Feb 5, 2014

Uh oh!

Kijewski commented Feb 5, 2014

Uh oh!

benpicco commented Feb 5, 2014

Uh oh!

LudwigKnuepfer commented Feb 6, 2014

Uh oh!

benpicco commented Feb 7, 2014

Uh oh!

LudwigKnuepfer commented Feb 7, 2014

Uh oh!

benpicco commented Feb 7, 2014

Uh oh!

LudwigKnuepfer commented Feb 7, 2014

Uh oh!

Kijewski commented Feb 7, 2014

Uh oh!

Kijewski commented Feb 10, 2014

Uh oh!

LudwigKnuepfer commented Feb 10, 2014

Uh oh!

benpicco commented Feb 11, 2014

Uh oh!

Kijewski commented Feb 11, 2014

Uh oh!

Kijewski commented Feb 12, 2014

Uh oh!

miri64 commented Feb 12, 2014

Uh oh!

Kijewski commented Feb 12, 2014

Uh oh!

miri64 commented Feb 12, 2014

Uh oh!

Kijewski commented Feb 12, 2014

Uh oh!

miri64 commented Feb 12, 2014

Uh oh!

benpicco commented Feb 12, 2014

Uh oh!