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

system: add libuv port for NuttX #362

Merged
merged 1 commit into from Aug 17, 2020
Merged

system: add libuv port for NuttX #362

merged 1 commit into from Aug 17, 2020

Conversation

spiriou
Copy link
Contributor

@spiriou spiriou commented Aug 12, 2020

Summary
From http://docs.libuv.org/en/v1.x/
"libuv is a multi-platform support library with a focus on asynchronous I/O. It was primarily developed for use by Node.js, but it’s also used by Luvit, Julia, pyuv, and others."

Initial port to NuttX from libuv v1.38.1.

Impact
None, disabled by default (CONFIG_LIBUV)

Testing
Select CONFIG_LIBUV and CONFIG_LIBUV_UNIT_TESTS. Then run "libuvtest" in nsh.
All CONFIG_LIBUV_* options must be enabled for all the tests to pass.
Validated on "sim" and "stm32f4discovery" targets.

@patacongo
Copy link
Contributor

We need to be careful with this. We have to handle third party code very differently: We cannot use the Apache license header and we have to document the use of third party software in the LICENSE file. See also https://www.apache.org/legal/resolved.html

Most people opt correctly to NOT explicitly include third party software in the repository, but rather to download it on demand and build time and patch it as needed.

@xiaoxiang781216
Copy link
Contributor

We need to be careful with this. We have to handle third party code very differently: We cannot use the Apache license header and we have to document the use of third party software in the LICENSE file. See also https://www.apache.org/legal/resolved.html

Most people opt correctly to NOT explicitly include third party software in the repository, but rather to download it on demand and build time and patch it as needed.

@papatience libuv is downloaded from the official location, the code in here is the porting layer written by @spiriou , so I think we don't need to mention license specially.

@spiriou spiriou force-pushed the libuv branch 2 times, most recently from 6bf180e to c268168 Compare August 12, 2020 16:17
@spiriou
Copy link
Contributor Author

spiriou commented Aug 12, 2020

@patacongo @xiaoxiang781216 indeed most of the code+unit-tests is downloaded from https://github.com/libuv/libuv/. I added in the patch the apache license (with the libuv license below) to protect the modifications to support NuttX on those files.

There are still few exceptions. Those files are issued from a copy/paste, and modified so they can work on NuttX. The modifications are too heavy to be managed with #ifdef #endif. What I did is reuse the libuv license, with apache license on top of it. Those files can be moved in the patch instead.
system/libuv/libuv/nuttx_async.c
system/libuv/libuv/nuttx_stream.c
system/libuv/libuv/nuttx_threadpool.c
system/libuv/libuv/nuttx.c
system/libuv/libuv/nuttx_tcp.c

system/libuv/libuv/nuttx_timer.c: that one is done from scratch so no license issue. It just implements the libuv API for that feature.

$(call DELDIR, $(LIBUV_UNPACKDIR))
$(call DELFILE, $(CONFIG_LIBUV_TARBALL))

include $(APPDIR)/Directory.mk
Copy link
Member

Choose a reason for hiding this comment

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

Is this really a "Directory" type Makefile? It looks like an Application type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this. I want to build the test executable (system/libuv/tests/Makefile) which uses "Application.mk", and also the libuv library (system/libuv/libuv/Makefile) that I declared as another "Application.mk". Is this the right approach ? How can I build both the test tool and the library in the same Makefile ?

Copy link
Member

Choose a reason for hiding this comment

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

You can do that with an Application.mk makefile as well.
The reason I'm saying this is because you are not using any of the prepared targets from Directory.mk and you are recreating targets that already exist in Application.mk.
For instance Application.mk's file already creates the .depend file in its depend target and it cleans it during its distclean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Makefiles should be fine now, I managed to remove the .depend hack.

Copy link
Member

@Ouss4 Ouss4 Aug 13, 2020

Choose a reason for hiding this comment

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

So you went with the Directory.mk? It's all right if it fits your need, however you need to add a .gitignore file and ignore the autogenerated Kconfig (this is what's causing the CI to fail).

@spiriou spiriou force-pushed the libuv branch 9 times, most recently from dca8c9d to 079325c Compare August 14, 2020 17:52
context:: $(LIBUV_UNPACKDIR)

clean::
$(call DELFILE, $(OBJS))
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? Application.mk already deletes object files. If the object files are generated else where then you need to extend clean::.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my understanding is CLEAN only removes "$(Q) rm -f $(OBJEXT) $(LIBEXT)", so ".o" and ".a".
It can be tricky to use VPATH here due to potential file name collisions between libuv/{libuv,tests} and libuv/libuv-1.38.1/.
So I added the files in CSRCS variable with absolute path. Hence the CLEAN target does not work anymore because the ".o" files are located in another folder, "libuv-1.38.1/src/[unix]".

If it's an issue, I can switch to VPATH and ensure there is no filename collision.

Copy link
Member

Choose a reason for hiding this comment

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

It's not an issue it's exactly how it should work. But are your $(LIBUV_UNPACKDIR)/src/*.o files being cleaned? As I see it, it cleans only at the libuv/libuv level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, all the objects are cleaned with this trick, even the ones not in libuv/ or tests/ folders like "./libuv-1.38.1/src/unix/nuttx_stream.path.to.nuttx.incubator-nuttx-apps.system.libuv.libuv.o".
OBJS is defined based on $(CSRCS:.c=$(SUFFIX)$(OBJEXT)) so it works fine.
How do you feel about this ?
What would be the correct solution to ensure all the objects are cleaned without extending the clean:: target ?

Copy link
Member

Choose a reason for hiding this comment

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

No. You are right.
The clean target called from Application only removes the files at the current level, it doesn't clean subdirectories, we need to extend it to clean any other file generated. The way you extended it to clean all OBJS does go through all the subdirectories.

Copy link
Contributor

Choose a reason for hiding this comment

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

The CLEAN operation does more than just remove object files. See definitions in tools/Config.mk

Copy link
Contributor

Choose a reason for hiding this comment

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

The CLEAN operation of tools/Config.mk is also over-ridden by architecture-specific operations that generate other kinds of files. See for example, tools/zds/Config.mk. If you simply remove the $(OBJS) in the sub-directory, the clean will be incomplete in some cases.

Copy link
Member

Choose a reason for hiding this comment

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

The clean target as called from the general Application.mk does the regular CLEAN. The application specific Makefile extends that to clean any other files that can not be reached by CLEAN.

Copy link
Member

@Ouss4 Ouss4 left a comment

Choose a reason for hiding this comment

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

LGTM. @xiaoxiang781216 @patacongo any concerns?

@spiriou spiriou force-pushed the libuv branch 3 times, most recently from 86d383a to c1da0da Compare August 16, 2020 20:36
@acassis acassis merged commit 18158ed into apache:master Aug 17, 2020
@btashton btashton added this to To-Add in Release Notes - 10.0.0 Oct 19, 2020
@btashton btashton moved this from To-Add to Added in Release Notes - 10.0.0 Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants