[PATCHES][DING-LIBS] Fix two distcheck issues with libini_config

[Stephen Gallagher]

Patch 0001: The unit tests for libini_config need to be able to validate
specific permissions on some sample data. However, 'make distcheck'
always removes the 'write' permissions on data in the $srcdir in order
to test that the build does not modify the contents of $srcdir. So we'll
solve this by moving the sample data being tested to the $builddir.

Patch 0002: There was an issue where, if the repo was cloned as root,
the sample data did not have the proper permissions and would fail the
checks. Previously, we had a hackish workaround where we would just
check to see if it matched the root-necessary permissions. Now we'll
just explicitly set them properly beforehand.

[Dmitri Pal]

On 05/11/2012 10:04 AM, Stephen Gallagher wrote:
>  Patch 0001: The unit tests for libini_config need to be able to
> validate
>  specific permissions on some sample data. However, 'make distcheck'
>  always removes the 'write' permissions on data in the $srcdir in order
>  to test that the build does not modify the contents of $srcdir. So we'll
>  solve this by moving the sample data being tested to the $builddir.
>
>  Patch 0002: There was an issue where, if the repo was cloned as root,
>  the sample data did not have the proper permissions and would fail the
>  checks. Previously, we had a hackish workaround where we would just
>  check to see if it matched the root-necessary permissions. Now we'll
>  just explicitly set them properly beforehand.
>
>
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/sssd-devel 
I do not like the first
patch.
I think it will fail when you build in tree. Have you checked?

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

[Dmitri Pal]

On 05/11/2012 04:21 PM, Dmitri Pal wrote:
>  On 05/11/2012 10:04 AM, Stephen Gallagher wrote:
> > Patch 0001: The unit tests for libini_config need to be able to validate
> > specific permissions on some sample data. However, 'make distcheck'
> > always removes the 'write' permissions on data in the $srcdir in order
> > to test that the build does not modify the contents of $srcdir. So we'll
> > solve this by moving the sample data being tested to the $builddir.
> >
> > Patch 0002: There was an issue where, if the repo was cloned as root,
> > the sample data did not have the proper permissions and would fail the
> > checks. Previously, we had a hackish workaround where we would just
> > check to see if it matched the root-necessary permissions. Now we'll
> > just explicitly set them properly beforehand.
> >
> >
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel(a)lists.fedorahosted.org
> > https://fedorahosted.org/mailman/listinfo/sssd-devel
>  I do not like the first patch.
>  I think it will fail when you build in tree. Have you checked?
>  
Ok. I checked it builds fine. However there are other places where the
same change needs to be implemented:

ini_config_ut.c:    srcdir = getenv("srcdir");
ini_parse_ut.c:    srcdir = getenv("srcdir");
ini_parse_ut.c:    srcdir = getenv("srcdir");

I am also not sure whether we need to change the build dir. This is used
to compare generated files that are created during the build with the
files that are stored in the source.

ini_parse_ut.c:    builddir = getenv("builddir");




>  -- 
>  Thank you,
>  Dmitri Pal
>
>  Sr. Engineering Manager IPA project,
>  Red Hat Inc.
>
>
>  -------------------------------
>  Looking to carve out IT costs?
>  www.redhat.com/carveoutcosts/
>
>
>
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/sssd-devel 

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

[Jan Zelený]

>  On 05/11/2012 04:21 PM, Dmitri Pal wrote:
>  > On 05/11/2012 10:04 AM, Stephen Gallagher wrote:
>  >> Patch 0001: The unit tests for libini_config need to be able to validate
>  >> specific permissions on some sample data. However, 'make distcheck'
>  >> always removes the 'write' permissions on data in the $srcdir in order
>  >> to test that the build does not modify the contents of $srcdir. So we'll
>  >> solve this by moving the sample data being tested to the $builddir.
>  >> 
>  >> Patch 0002: There was an issue where, if the repo was cloned as root,
>  >> the sample data did not have the proper permissions and would fail the
>  >> checks. Previously, we had a hackish workaround where we would just
>  >> check to see if it matched the root-necessary permissions. Now we'll
>  >> just explicitly set them properly beforehand.
>  >> 
>  >> 
>  >> _______________________________________________
>  >> sssd-devel mailing list
>  >> sssd-devel(a)lists.fedorahosted.org
>  >> https://fedorahosted.org/mailman/listinfo/sssd-devel
>  > 
>  > I do not like the first patch.
>  > I think it will fail when you build in tree. Have you checked?
>  
>  Ok. I checked it builds fine. However there are other places where the
>  same change needs to be implemented:
>  
>  ini_config_ut.c:    srcdir = getenv("srcdir");
>  ini_parse_ut.c:    srcdir = getenv("srcdir");
>  ini_parse_ut.c:    srcdir = getenv("srcdir"); 
I don't think we need to do that since those instances are for different files. 
The only file affected by this change is foo.conf and Stephen correctly changed 
all instances of the srcdir for that file.

>  I am also not sure whether we need to change the build dir. This is
> used
>  to compare generated files that are created during the build with the
>  files that are stored in the source.
>  
>  ini_parse_ut.c:    builddir = getenv("builddir"); 
I'm not sure what do you mean. I think the test is ok, since it copies the file 
to a different dir, tests it there and then removes it.

ACK from me, just please consider one small change - use memcpy() instead of 
sprintf(), there is no need for sprintf() since the string is static. I'm 
attaching proposed change.

Thanks
Jan

[Stephen Gallagher]

On Mon, 2012-05-14 at 09:19 +0200, Jan Zelený wrote:
>  > On 05/11/2012 04:21 PM, Dmitri Pal wrote:
>  > > On 05/11/2012 10:04 AM, Stephen Gallagher wrote:
>  > >> Patch 0001: The unit tests for libini_config need to be able to validate
>  > >> specific permissions on some sample data. However, 'make
> distcheck'
>  > >> always removes the 'write' permissions on data in the $srcdir in
> order
>  > >> to test that the build does not modify the contents of $srcdir. So
> we'll
>  > >> solve this by moving the sample data being tested to the $builddir.
>  > >> 
>  > >> Patch 0002: There was an issue where, if the repo was cloned as root,
>  > >> the sample data did not have the proper permissions and would fail the
>  > >> checks. Previously, we had a hackish workaround where we would just
>  > >> check to see if it matched the root-necessary permissions. Now we'll
>  > >> just explicitly set them properly beforehand.
>  > >> 
>  > >> 
>  > >> _______________________________________________
>  > >> sssd-devel mailing list
>  > >> sssd-devel(a)lists.fedorahosted.org
>  > >> https://fedorahosted.org/mailman/listinfo/sssd-devel
>  > > 
>  > > I do not like the first patch.
>  > > I think it will fail when you build in tree. Have you checked?
>  > 
>  > Ok. I checked it builds fine. However there are other places where the
>  > same change needs to be implemented:
>  > 
>  > ini_config_ut.c:    srcdir = getenv("srcdir");
>  > ini_parse_ut.c:    srcdir = getenv("srcdir");
>  > ini_parse_ut.c:    srcdir = getenv("srcdir");
>  
>  I don't think we need to do that since those instances are for different files. 
>  The only file affected by this change is foo.conf and Stephen correctly changed 
>  all instances of the srcdir for that file.
>  
>  > I am also not sure whether we need to change the build dir. This is used
>  > to compare generated files that are created during the build with the
>  > files that are stored in the source.
>  > 
>  > ini_parse_ut.c:    builddir = getenv("builddir"); 
For what it's worth, this operation is guaranteed to be useless. In all
versions of autoconf that support $builddir, it's actually hard-coded to
be './'

It's mostly there as a convenience variable to make the code easier to
read.

>  
>  I'm not sure what do you mean. I think the test is ok, since it copies the file 
>  to a different dir, tests it there and then removes it.
>  
>  ACK from me, just please consider one small change - use memcpy() instead of 
>  sprintf(), there is no need for sprintf() since the string is static. I'm 
>  attaching proposed change. 

Ack.

[Dmitri Pal]

On 05/14/2012 07:13 AM, Stephen Gallagher wrote:
>  On Mon, 2012-05-14 at 09:19 +0200, Jan Zelený wrote:
> >> On 05/11/2012 04:21 PM, Dmitri Pal wrote:
> >>> On 05/11/2012 10:04 AM, Stephen Gallagher wrote:
> >>>> Patch 0001: The unit tests for libini_config need to be able to validate
> >>>> specific permissions on some sample data. However, 'make
> distcheck'
> >>>> always removes the 'write' permissions on data in the $srcdir in
> order
> >>>> to test that the build does not modify the contents of $srcdir. So
> we'll
> >>>> solve this by moving the sample data being tested to the $builddir.
> >>>>
> >>>> Patch 0002: There was an issue where, if the repo was cloned as root,
> >>>> the sample data did not have the proper permissions and would fail the
> >>>> checks. Previously, we had a hackish workaround where we would just
> >>>> check to see if it matched the root-necessary permissions. Now we'll
> >>>> just explicitly set them properly beforehand.
> >>>>
> >>>>
> >>>> _______________________________________________
> >>>> sssd-devel mailing list
> >>>> sssd-devel(a)lists.fedorahosted.org
> >>>> https://fedorahosted.org/mailman/listinfo/sssd-devel
> >>> I do not like the first patch.
> >>> I think it will fail when you build in tree. Have you checked?
> >> Ok. I checked it builds fine. However there are other places where the
> >> same change needs to be implemented:
> >>
> >> ini_config_ut.c:    srcdir = getenv("srcdir");
> >> ini_parse_ut.c:    srcdir = getenv("srcdir");
> >> ini_parse_ut.c:    srcdir = getenv("srcdir");
> > I don't think we need to do that since those instances are for different files. 
> > The only file affected by this change is foo.conf and Stephen correctly changed 
> > all instances of the srcdir for that file. 
foo.conf is no different that other test config files that reside in the
ini.d directory. I do not understand why the change needed for one and
not for the others. Can you please elaborate?

> >> I am also not sure whether we need to change the build dir.
> This is used
> >> to compare generated files that are created during the build with the
> >> files that are stored in the source.
> >>
> >> ini_parse_ut.c:    builddir = getenv("builddir");
>  For what it's worth, this operation is guaranteed to be useless. In all
>  versions of autoconf that support $builddir, it's actually hard-coded to
>  be './'
>
>  It's mostly there as a convenience variable to make the code easier to
>  read.
>
> > I'm not sure what do you mean. I think the test is ok, since it copies the file 
> > to a different dir, tests it there and then removes it.
> >
> > ACK from me, just please consider one small change - use memcpy() instead of 
> > sprintf(), there is no need for sprintf() since the string is static. I'm 
> > attaching proposed change.
>
>  Ack.
>
>
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/sssd-devel 

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

[Jan Zelený]

>  On 05/14/2012 07:13 AM, Stephen Gallagher wrote:
>  > On Mon, 2012-05-14 at 09:19 +0200, Jan Zelený wrote:
>  >>> On 05/11/2012 04:21 PM, Dmitri Pal wrote:
>  >>>> On 05/11/2012 10:04 AM, Stephen Gallagher wrote:
>  >>>>> Patch 0001: The unit tests for libini_config need to be able to
>  >>>>> validate specific permissions on some sample data. However,
> 'make
>  >>>>> distcheck' always removes the 'write' permissions on
> data in the
>  >>>>> $srcdir in order to test that the build does not modify the
> contents
>  >>>>> of $srcdir. So we'll solve this by moving the sample data being
>  >>>>> tested to the $builddir.
>  >>>>> 
>  >>>>> Patch 0002: There was an issue where, if the repo was cloned as
> root,
>  >>>>> the sample data did not have the proper permissions and would fail
>  >>>>> the checks. Previously, we had a hackish workaround where we would
>  >>>>> just check to see if it matched the root-necessary permissions. Now
>  >>>>> we'll just explicitly set them properly beforehand.
>  >>>>> 
>  >>>>> 
>  >>>>> _______________________________________________
>  >>>>> sssd-devel mailing list
>  >>>>> sssd-devel(a)lists.fedorahosted.org
>  >>>>> https://fedorahosted.org/mailman/listinfo/sssd-devel
>  >>>> 
>  >>>> I do not like the first patch.
>  >>>> I think it will fail when you build in tree. Have you checked?
>  >>> 
>  >>> Ok. I checked it builds fine. However there are other places where the
>  >>> same change needs to be implemented:
>  >>> 
>  >>> ini_config_ut.c:    srcdir = getenv("srcdir");
>  >>> ini_parse_ut.c:    srcdir = getenv("srcdir");
>  >>> ini_parse_ut.c:    srcdir = getenv("srcdir");
>  >> 
>  >> I don't think we need to do that since those instances are for different
>  >> files. The only file affected by this change is foo.conf and Stephen
>  >> correctly changed all instances of the srcdir for that file.
>  
>  foo.conf is no different that other test config files that reside in the
>  ini.d directory. I do not understand why the change needed for one and
>  not for the others. Can you please elaborate? 
Because only foo.conf serves you as file for testing of the access permissions. 
Basically make distcheck led to another set of access rights the file could 
have, leading to failure of the test. The solution was to move this file to 
builddir during make distcheck and change access rights there. There is no 
need to do this for other config files.

Jan

>  >>> I am also not sure whether we need to change the build
> dir. This is
>  >>> used to compare generated files that are created during the build with
>  >>> the files that are stored in the source.
>  >>> 
>  >>> ini_parse_ut.c:    builddir = getenv("builddir");
>  > 
>  > For what it's worth, this operation is guaranteed to be useless. In all
>  > versions of autoconf that support $builddir, it's actually hard-coded to
>  > be './'
>  > 
>  > It's mostly there as a convenience variable to make the code easier to
>  > read.
>  > 
>  >> I'm not sure what do you mean. I think the test is ok, since it copies
>  >> the file to a different dir, tests it there and then removes it.
>  >> 
>  >> ACK from me, just please consider one small change - use memcpy()
>  >> instead of sprintf(), there is no need for sprintf() since the string
>  >> is static. I'm attaching proposed change.
>  > 
>  > Ack.

[Dmitri Pal]

On 05/14/2012 09:31 AM, Jan Zelený wrote:
> > On 05/14/2012 07:13 AM, Stephen Gallagher wrote:
> >> On Mon, 2012-05-14 at 09:19 +0200, Jan Zelený wrote:
> >>>> On 05/11/2012 04:21 PM, Dmitri Pal wrote:
> >>>>> On 05/11/2012 10:04 AM, Stephen Gallagher wrote:
> >>>>>> Patch 0001: The unit tests for libini_config need to be able to
> >>>>>> validate specific permissions on some sample data. However,
> 'make
> >>>>>> distcheck' always removes the 'write' permissions on
> data in the
> >>>>>> $srcdir in order to test that the build does not modify the
> contents
> >>>>>> of $srcdir. So we'll solve this by moving the sample data
> being
> >>>>>> tested to the $builddir.
> >>>>>>
> >>>>>> Patch 0002: There was an issue where, if the repo was cloned as
> root,
> >>>>>> the sample data did not have the proper permissions and would
> fail
> >>>>>> the checks. Previously, we had a hackish workaround where we
> would
> >>>>>> just check to see if it matched the root-necessary permissions.
> Now
> >>>>>> we'll just explicitly set them properly beforehand.
> >>>>>>
> >>>>>>
> >>>>>> _______________________________________________
> >>>>>> sssd-devel mailing list
> >>>>>> sssd-devel(a)lists.fedorahosted.org
> >>>>>> https://fedorahosted.org/mailman/listinfo/sssd-devel
> >>>>> I do not like the first patch.
> >>>>> I think it will fail when you build in tree. Have you checked?
> >>>> Ok. I checked it builds fine. However there are other places where the
> >>>> same change needs to be implemented:
> >>>>
> >>>> ini_config_ut.c:    srcdir = getenv("srcdir");
> >>>> ini_parse_ut.c:    srcdir = getenv("srcdir");
> >>>> ini_parse_ut.c:    srcdir = getenv("srcdir");
> >>> I don't think we need to do that since those instances are for different
> >>> files. The only file affected by this change is foo.conf and Stephen
> >>> correctly changed all instances of the srcdir for that file.
> > foo.conf is no different that other test config files that reside in the
> > ini.d directory. I do not understand why the change needed for one and
> > not for the others. Can you please elaborate?
>  Because only foo.conf serves you as file for testing of the access permissions. 
>  Basically make distcheck led to another set of access rights the file could 
>  have, leading to failure of the test. The solution was to move this file to 
>  builddir during make distcheck and change access rights there. There is no 
>  need to do this for other config files.
>
>  Jan
>
> >>>> I am also not sure whether we need to change the build dir. This is
> >>>> used to compare generated files that are created during the build with
> >>>> the files that are stored in the source.
> >>>>
> >>>> ini_parse_ut.c:    builddir = getenv("builddir");
> >> For what it's worth, this operation is guaranteed to be useless. In all
> >> versions of autoconf that support $builddir, it's actually hard-coded to
> >> be './'
> >>
> >> It's mostly there as a convenience variable to make the code easier to
> >> read.
> >>
> >>> I'm not sure what do you mean. I think the test is ok, since it copies
> >>> the file to a different dir, tests it there and then removes it.
> >>>
> >>> ACK from me, just please consider one small change - use memcpy()
> >>> instead of sprintf(), there is no need for sprintf() since the string
> >>> is static. I'm attaching proposed change.
> >> Ack. 
Ah OK. I might have misread a part of the patch then.

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

[Stephen Gallagher]

On Mon, 2012-05-14 at 08:41 -0400, Dmitri Pal wrote:

>  
>  foo.conf is no different that other test config files that reside in
>  the ini.d directory. I do not understand why the change needed for one
>  and not for the others. Can you please elaborate? 
Foo.conf is a special case. It's the only file whose specific file
permissions are tested during the unit test run. This causes issues when
running tests in 'make distcheck' because of a peculiarity in the design
of distcheck.

One of the tests that distcheck performs is to ensure that no part of
the build process modifies the $srcdir. Distcheck takes the following
steps:
1) run 'make dist'
2) Untar the new tarball into a fresh location (this is to ensure that
all files needed for the build have been properly added to the tarball)
3) Change to the new untarred location and run 'chmod -R a-w'. This
makes EVERY file in the checkout unwritable, by changing its
permissions. This means that if something in the build tries to modify
these files, the build will fail (and you'll have to go figure out why
you're trying to modify files in $srcdir instead of $builddir).
3) Now it runs 'make all' and 'make check' and 'make
DESTDIR=tmplocation
install' to verify that those all work without issue.

Foo.conf isn't modified, but it DOES need to have a set of specific
permissions in order for its test to succeed. So in order to avoid
having 'make distcheck' alter them on us, we rig up the build system so
it copies foo.conf into the $builddir and uses it from there (where it's
safe from the 'chmod -R a-w'). Thus, we are able to properly validate
it.