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.
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@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?
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@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@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
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@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
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@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.
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@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@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
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@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.
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@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.
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.
sssd-devel@lists.fedorahosted.org
-
Dmitri Pal -
Jan Zelený -
Stephen Gallagher