Re: [Minios-devel] [UNIKRAFT/CLICK PATCH v2 01/11] Initial public release: basic unikraft files

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

Hi Simon,

Felipe and I decided to split the work on those. I'll comment on what 
I'll fix.
On 6/5/19 1:13 PM, Simon Kuenzer wrote:
> Hey all,
>
> I have actually some comments about this patch. See my comments inline.
>
> On 05.06.19 10:56, Costin Lupu wrote:
> > Reviewed-by: Costin Lupu <costin.lupu@xxxxxxxxx>
> >
> > On 6/5/19 10:35 AM, Florian Schmidt wrote:
> > > --- /dev/null
> > > +++ b/Config.uk
> > > @@ -0,0 +1,86 @@
> > > +### Invisible option for dependencies
> > > +config APP_DEPENDENCIES
> > > +    bool
> > > +    default y
> > > +
> > > +menuconfig LIBCLICK
> > > +    bool "The Click modular router"
> > > +    depends on HAVE_LIBC
> > > +    select LIBLWIP
> > > +    default y
> It is probably better if you select LIBNEWLIB instead of checking if a 
> LIBC is there. If we add more libc's later, we anyway need to check if 
> it compiles. newlib is the working configuration.
OK, I'll select LIBNEWLIB. I also realized that click needs ukunistd, so 
I'll also add select UKUNISTD.

> > > --- /dev/null
> > > +++ b/Makefile
> > > @@ -0,0 +1,9 @@
> > > +UK_ROOT ?= $(PWD)/../unikraft
> > > +UK_LIBS ?= $(PWD)/..
> > > +LIBS := $(UK_LIBS)/newlib:$(UK_LIBS)/lwip
> > > +
> So far, we had the following default definition for UK_ROOT and UK_LIBS:
>   UK_ROOT ?= $(PWD)/../../unikraft
>   UK_LIBS ?= $(PWD)/../../libs
> For simplicity we should use the as we do with the applications 
> `helloworld` and `httpreply` so that user can expect the same default 
> directory structure.

I'll change the paths accordingly.

> > > +################################################################################ 
> > >
> > > +# Check requirements: Click requires netdev access, so LWIP must not 
> > > attach them
> > > +################################################################################ 
> > >
> > > +ifeq ($(CONFIG_LWIP_AUTOIFACE),y)
> > > +$(error Click cannot be built with LWIP option "Automatically attach 
> > > netifs"! Please disable)
> > > +endif
> Could you add a comment why you are doing this with an error?: KConfig 
> unfortunately does not support to `unselect` an configuration option. I 
> hope problems like this can be fixed by updating to a recent KConfig 
> system.

I'll add a comment.

The rest is up to Felipe.

Cheers,
Florian

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel