[PATCH] ELAPI Adding concept of the targets

[Dmitri Pal]

See the patch comment.
The wiki page will be really helpful during review: 
https://fedorahosted.org/sssd/wiki/WikiPage/ELAPIInterface

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/14/2009 04:50 PM, Dmitri Pal wrote:
> See the patch comment.
> The wiki page will be really helpful during review: 
> https://fedorahosted.org/sssd/wiki/WikiPage/ELAPIInterface
>
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

Nack
Please use a shorter struct name than "elapi_target_pass_in_data".
Keeping line lengths to < 80 characters gets difficult.
struct elapi_recv_data or something similar would be sufficient.

Similarly, _ctx is preferred in the SSSD over _context (such as
elapi_target_ctx instead of elapi_target_context)

elapi_internal_target_cleanup_handler() would be better as
elapi_target_free()

Typo in elapi_internal_add_sink: "Sink refernce before call" should be
"Sink reference before call"

In elapi_internal_create_target, you make reference to relaxing the
checks for adding the sinks and being satisfied with at least one sink
in the list. I think a safer approach would be to define sinks with an
attribute of "Mandatory" and "Preferred". If a sink was mandatory and
failed to load, it should be an error condition. If a sink was Preferred
and failed to load, the ELAPI should continue on. If all sinks were
"Preferred" and none loaded, this should also be an error condition.

EALPI_EVT_* macros were probably meant to be ELAPI_EVT_*

When creating a new directory, you also need to git add a new m4
directory (I usually create the empty file m4/.dir to accomplish this)

Your unit tests do not build in a parallel build tree. You need to make
sure that you use the $(srcdir) and $(builddir) variables correctly. Any
path that needs to be relative to the original sources must use
$(srcdir) and any path that needs to be relative to built binaries or
generated scripts needs to use $(builddir)

So, you need to ensure that your elapi_ut_LDADD entries use $(builddir)
to find the libraries to link against, and you need to ensure that the
- -D macros you're passing to the compiler refer to $(srcdir). The
AM_CPPFLAGS are correctly using $(srcdir) as-is.

- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqJsRQACgkQeiVVYja6o6MyKACfSaC+EJ/4kPIULhaXq8Cbovhp
> 4yAAoIowmsoelojYbT42p0F3vEvCvOSK
> =cdLf
> -----END PGP SIGNATURE-----

[Dmitri Pal]

Hi Steven,

Thanks for review!
Comments and questions below.

Stephen Gallagher wrote:
> On 08/14/2009 04:50 PM, Dmitri Pal wrote:
> > See the patch comment.
> > The wiki page will be really helpful during review:
> > https://fedorahosted.org/sssd/wiki/WikiPage/ELAPIInterface
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://fedorahosted.org/mailman/listinfo/sssd-devel
>
> Nack
> Please use a shorter struct name than "elapi_target_pass_in_data".
> Keeping line lengths to < 80 characters gets difficult.
> struct elapi_recv_data or something similar would be sufficient.
>
> Similarly, _ctx is preferred in the SSSD over _context (such as
> elapi_target_ctx instead of elapi_target_context)
>
> elapi_internal_target_cleanup_handler() would be better as
> elapi_target_free()

I will see what I can do about the names.
I think that it would make sense to have a separate patch that deals
with names then if you are concerned with things getting too long.
I have my own pattern of naming. If it does not fly I need to change the
pattern.

Here is what I can do:
a) Remove the word internal at all from the internal functions. May be
it is a not so great idea to have it in the first place
b) Replace context with "ctx"
c) Shorten or abbreviate names of the functions and structures to make
them shorter but still readable.

I think it should be a separate "naming correction patch". I can add it
to this one but it might be harder for you to review.
Your call. I do not have a preference but in the wake of 0.5 release I
do not want to create addition work for you.
Since this code is not called by anyone yet there is no harm to log a
ticket to align naming and move on.
> Typo in elapi_internal_add_sink: "Sink refernce before call" should be
> "Sink reference before call"

Will fix that one.
> In elapi_internal_create_target, you make reference to relaxing the
> checks for adding the sinks and being satisfied with at least one sink
> in the list. I think a safer approach would be to define sinks with an
> attribute of "Mandatory" and "Preferred". If a sink was mandatory and
> failed to load, it should be an error condition. If a sink was Preferred
> and failed to load, the ELAPI should continue on. If all sinks were
> "Preferred" and none loaded, this should also be an error condition.

May be, but I am not there yet. One of the issues is that a sink can be 
mandatory for one target and optional for another.
I was thinking about this a lot and do not have a good approach yet.
I need to think more about it, this is why there are placeholders in the
code in the areas related to loading sinks.
> EALPI_EVT_* macros were probably meant to be ELAPI_EVT_*

Typo - will fix.
> When creating a new directory, you also need to git add a new m4
> directory (I usually create the empty file m4/.dir to accomplish this)

I thought I did. I definitely created it and thought that I added it
commit.
I do not know what happened. I will try again.
> Your unit tests do not build in a parallel build tree.

What do you mean they do not build in parallel build tree?
The elapi_test is supposed to be built after ELAPI.
> You need to make
> sure that you use the $(srcdir) and $(builddir) variables correctly.

My library source is one level above test and this is what I refer to.
This is why when I build a special instance of the libelapi for testing
(being inside elapi_test directory) I reference most of the code as "../"
This all builds and works for me correctly.
> Any
> path that needs to be relative to the original sources must use
> $(srcdir) and any path that needs to be relative to built binaries or
> generated scripts needs to use $(builddir)

Yes and this is what happens. My builddir for the test is
elapi_test and $(source) is the same but most of the source code that I
need to rebuild is actually a level above.
I do not see what is wrong and what you want me to change?
> So, you need to ensure that your elapi_ut_LDADD entries use $(builddir)
> to find the libraries to link against,

It does.
> and you need to ensure that the
> -D macros you're passing to the compiler refer to $(srcdir).

As I said the $(source) is elapi_test when a special instance of the
library is built but the code is one level above.
> The
> AM_CPPFLAGS are correctly using $(srcdir) as-is.

Thank you for review.
a) Please explain what you want me to do with naming (ticket + sep patch
or jam in this one)
b) Please explain what you want me to do with build. So I do not
understand what is wrong.

I plan to fix:
1) Typo in "reference"
2) Fix macro names
3) Add .dir to commit
4) Anything that comes from your follow up comments about items a) and
b) above.

Thanks
Dmitri

_______________________________________________
sssd-devel mailing list
[e-mail]
https://fedorahosted.org/mailman/listinfo/sssd-devel

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/17/2009 04:48 PM, Dmitri Pal wrote:
> Hi Steven,
>
> Thanks for review!
> Comments and questions below.
>
> Stephen Gallagher wrote:
> > On 08/14/2009 04:50 PM, Dmitri Pal wrote:
> > > See the patch comment.
> > > The wiki page will be really helpful during review:
> > > https://fedorahosted.org/sssd/wiki/WikiPage/ELAPIInterface
> > >
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > Nack
> > Please use a shorter struct name than "elapi_target_pass_in_data".
> > Keeping line lengths to < 80 characters gets difficult.
> > struct elapi_recv_data or something similar would be sufficient.
> >
> > Similarly, _ctx is preferred in the SSSD over _context (such as
> > elapi_target_ctx instead of elapi_target_context)
> >
> > elapi_internal_target_cleanup_handler() would be better as
> > elapi_target_free()
>
> I will see what I can do about the names.
> I think that it would make sense to have a separate patch that deals
> with names then if you are concerned with things getting too long.
> I have my own pattern of naming. If it does not fly I need to change the
> pattern.

I can live with a separate patch for names.
> Here is what I can do:
> a) Remove the word internal at all from the internal functions. May be
> it is a not so great idea to have it in the first place
> b) Replace context with "ctx"
> c) Shorten or abbreviate names of the functions and structures to make
> them shorter but still readable.
>
> I think it should be a separate "naming correction patch". I can add it
> to this one but it might be harder for you to review.
> Your call. I do not have a preference but in the wake of 0.5 release I
> do not want to create addition work for you.
> Since this code is not called by anyone yet there is no harm to log a
> ticket to align naming and move on.
> > Typo in elapi_internal_add_sink: "Sink refernce before call" should be
> > "Sink reference before call"
>
> Will fix that one.
> > In elapi_internal_create_target, you make reference to relaxing the
> > checks for adding the sinks and being satisfied with at least one sink
> > in the list. I think a safer approach would be to define sinks with an
> > attribute of "Mandatory" and "Preferred". If a sink was mandatory and
> > failed to load, it should be an error condition. If a sink was Preferred
> > and failed to load, the ELAPI should continue on. If all sinks were
> > "Preferred" and none loaded, this should also be an error condition.
>
> May be, but I am not there yet. One of the issues is that a sink can be 
> mandatory for one target and optional for another.
> I was thinking about this a lot and do not have a good approach yet.
> I need to think more about it, this is why there are placeholders in the
> code in the areas related to loading sinks.

Sorry, I should have mentioned that was not a review comment, merely a
recommendation on a question I saw in the comments.
> > EALPI_EVT_* macros were probably meant to be ELAPI_EVT_*
>
> Typo - will fix.
> > When creating a new directory, you also need to git add a new m4
> > directory (I usually create the empty file m4/.dir to accomplish this)
>
> I thought I did. I definitely created it and thought that I added it
> commit.
> I do not know what happened. I will try again.
> > Your unit tests do not build in a parallel build tree.
>
> What do you mean they do not build in parallel build tree?
> The elapi_test is supposed to be built after ELAPI.
> > You need to make
> > sure that you use the $(srcdir) and $(builddir) variables correctly.
>
> My library source is one level above test and this is what I refer to.
> This is why when I build a special instance of the libelapi for testing
> (being inside elapi_test directory) I reference most of the code as "../"
> This all builds and works for me correctly.
> > Any
> > path that needs to be relative to the original sources must use
> > $(srcdir) and any path that needs to be relative to built binaries or
> > generated scripts needs to use $(builddir)
>
> Yes and this is what happens. My builddir for the test is
> elapi_test and $(source) is the same but most of the source code that I
> need to rebuild is actually a level above.
> I do not see what is wrong and what you want me to change?

With automake, there are two variables that are automatically populated:
$(srcdir) and $(builddir). These refer to the original path of the
source files and to the output path of the built files. If you are
building in-tree, $(srcdir) == $(builddir). However, in order to be
fully GNU-compliant, we also support building in what are referred to as
parallel build trees. This means that you run configure from a directory
outside the normal source tree, and all configure output, compiler
output and completed binaries are built into this new build tree instead
of in the original source tree. This is done for two reasons: 1) During
development, it's easier to ensure that you don't accidentally commit
generated files alongside code, because all generated files (after
autoreconf, anyway) are built in a different directory. 2) Many release
engineering systems will use a single source checkout to build multiple
releases. They may mount the source checkout by NFS on several different
machines and build them from different architectures.

An illustration (e.g. How Steve Builds the SSSD):
I check out the latest code into the 'sssd' directory.
cd sssd
mkdir x86_64
cd x86_64
../configure <configure arguments>

Notice the ../configure (instead of ./configure). The output from
configure will generate a new build tree inside the x86_64 subdirectory.
This directory will have its own set of common, replace, server and
sss_client directories, each with the Makefile output by configure. Now
I can run 'make' from within the x86_64 directory and it will build the
temporary objects, final objects, manpages and scripts in the x86_64
tree instead of the main source tree.

So in this example, if I was building the elapi_test, my $(srcdir)
variable would be /sssd/common/elapi/elapi_test and my $(builddir)
variable would be /sssd/x86_64/common/elapi/elapi_test.

So, in order to link against a built library, my link path needs to be
relative to the $(builddir) and not the $(srcdir). But if I want to add
an include path, it needs to use the $(srcdir) variable so that it can
find it in the original source path.

The easiest way I've found to make sure it's all working is to always
perform parallel builds and never build in-tree. Plus, it makes cleanup
a snap (rm -Rf x86_64 is more reliable than 'make clean' or 'make
distclean' if you've been playing around with the contents of Makefile.am)
> > So, you need to ensure that your elapi_ut_LDADD entries use $(builddir)
> > to find the libraries to link against,
>
> It does.
> > and you need to ensure that the
> > -D macros you're passing to the compiler refer to $(srcdir).
>
> As I said the $(source) is elapi_test when a special instance of the
> library is built but the code is one level above.
> > The
> > AM_CPPFLAGS are correctly using $(srcdir) as-is.
>
> Thank you for review.
> a) Please explain what you want me to do with naming (ticket + sep patch
> or jam in this one)
> b) Please explain what you want me to do with build. So I do not
> understand what is wrong.
>
> I plan to fix:
> Typo in "reference"
> Fix macro names
> Add .dir to commit
> Anything that comes from your follow up comments about items a) and
>
> b) above.
>
> Thanks
> Dmitri
>
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqKnDwACgkQeiVVYja6o6PyRwCgkDmleHecHf0CTJXyMFLaKQII
> jzMAnRakn5EmPql14mcaQOheP/u5apTE
> =R4Ac
> -----END PGP SIGNATURE-----

[Dmitri Pal]

> > I plan to fix:
> > Typo in "reference"

Done
> > Fix macro names

Done
> > Add .dir to commit

Done
> > Anything that comes from your follow up comments about items a) and
> >
> > b) above.

Fixed the makefile.
Created a ticket to address re-naming issues.

The other patch that was done on top of this one remains unchanged.
> > Thanks
> > Dmitri
> > sssd-devel mailing list
> > [e-mail]
> > https://fedorahosted.org/mailman/listinfo/sssd-devel

_______________________________________________
sssd-devel mailing list
[e-mail]
https://fedorahosted.org/mailman/listinfo/sssd-devel

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/18/2009 03:35 PM, Dmitri Pal wrote:
> > > I plan to fix:
> > > Typo in "reference"
>
> Done
> > > Fix macro names
>
> Done
> > > Add .dir to commit
>
> Done
> > > Anything that comes from your follow up comments about items a) and
> > >
> > > b) above.
>
> Fixed the makefile.
> Created a ticket to address re-naming issues.
>
> The other patch that was done on top of this one remains unchanged.
> > > Thanks
> > > Dmitri
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://fedorahosted.org/mailman/listinfo/sssd-devel
>
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel
>
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

Nack. Please TRY building in a parallel build directory.

'make check' fails because -DELAPI_DEFAULT_CONFIG_DIR points to '.'
instead of $(srcdir). Same for -DELAPI_DEFAULT_CONFIG_APP_DIR

- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqMA+QACgkQeiVVYja6o6P6DwCglcH9gs2lPu3oeOf3343Evt41
> 0RcAoIb0ukbrPjifNZy+QfIlQ6VE7LH7
> =ko8A
> -----END PGP SIGNATURE-----

[Dmitri Pal]

> Nack. Please TRY building in a parallel build directory.
>
> 'make check' fails because -DELAPI_DEFAULT_CONFIG_DIR points to '.'
> instead of $(srcdir). Same for -DELAPI_DEFAULT_CONFIG_APP_DIR

Changed the flags, tried building "common" in parallel tree - works for me.
Patch attached.
Other patch is not affected.
_______________________________________________
sssd-devel mailing list
[e-mail]
https://fedorahosted.org/mailman/listinfo/sssd-devel

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/19/2009 07:09 PM, Dmitri Pal wrote:
> > Nack. Please TRY building in a parallel build directory.
> >
> > 'make check' fails because -DELAPI_DEFAULT_CONFIG_DIR points to '.'
> > instead of $(srcdir). Same for -DELAPI_DEFAULT_CONFIG_APP_DIR
>
> Changed the flags, tried building "common" in parallel tree - works for me.
> Patch attached.
> Other patch is not affected.
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel
>
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

Ack and pushed to master.

- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqNqGwACgkQeiVVYja6o6MVvACfVTCbE6HHvpSoI4QH8wscsPeF
> VzUAnAk5jydONB3ox/YouN/J35QjyZ5q
> =JcBo
> -----END PGP SIGNATURE-----