By default, Bugzilla does not search the list of RESOLVED bugs.
You can force it to do so by putting the upper-case word ALL in front of your search query, e.g.: ALL tdelibs
We recommend searching for bugs this way, as you may discover that your bug has already been resolved and fixed in a later release.
Bug 2695 - build fails on vcardparser tests
Summary: build fails on vcardparser tests
Status: RESOLVED FIXED
Alias: None
Product: TDE
Classification: Unclassified
Component: tdelibs (show other bugs)
Version: R14.1.x [Trinity]
Hardware: Other Linux
: P5 normal
Assignee: Slávek Banko
URL:
Depends on:
Blocks: R14.0.4
  Show dependency treegraph
 
Reported: 2016-09-21 08:52 CDT by deloptes
Modified: 2016-09-27 13:27 CDT (History)
4 users (show)

See Also:
Compiler Version:
TDE Version String:
Application Version:
Application Name:


Attachments
Fix build dependencies in tdelibs/tdeabc/vcardparser/CMakeLists.txt (4.24 KB, application/octet_stream)
2016-09-21 08:52 CDT, deloptes
Details
Fix FTBFS on VCardParser tests (2.02 KB, patch)
2016-09-26 17:58 CDT, Slávek Banko
Details | Diff
new patch (2.02 KB, application/octet_stream)
2016-09-27 04:18 CDT, Michele Calgaro
Details
Fundamentally new patch :) (2.51 KB, patch)
2016-09-27 07:43 CDT, Slávek Banko
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description deloptes 2016-09-21 08:52:28 CDT
Created attachment 2694 [details]
Fix build dependencies in tdelibs/tdeabc/vcardparser/CMakeLists.txt

I pulled the latest git and tried to rebuild, then it failed on testing.
Inspecting the issue I found out it was build dependency in the CMake file.

Here is a patch for it.
A part in the patch is just cosmetics.
Comment 1 deloptes 2016-09-21 08:57:36 CDT
CMake Warning (dev) at tdeabc/vcardparser/CMakeLists.txt:93 (add_custom_target):
  Policy CMP0037 is not set: Target names should not be reserved and should
  match a validity pattern.  Run "cmake --help-policy CMP0037" for policy
  details.  Use the cmake_policy command to set the policy and suppress this
  warning.

  The target name "test" is reserved or not valid for certain CMake features,
  such as generator expressions, and may result in undefined behavior.
This warning is for project developers.  Use -Wno-dev to suppress it.


pfui - learned something once again
Comment 2 Michele Calgaro 2016-09-23 09:16:07 CDT
I just pulled last sources from R14.1.x (tde and packaging) on debian/stretch and made a new clean rebuild from scratch (not only tdelibs but all packages required). No building issues whatsoever.
Are you using some additional local patches?
Comment 3 deloptes 2016-09-24 17:23:48 CDT
Someone else might try to reproduce. Are you sure the other patches are in. In my original patch I skipped the test target. I was hoping Fat-Zer will do some work as promised earlier, so that we may work in structured way.

This patch might be inappropriate for this bug as it is about the testing.

Sorry, I get more organized in terms of TDE recently. I hope I'll get much better in future.
Comment 4 Michele Calgaro 2016-09-24 23:25:38 CDT
> I just pulled last sources from R14.1.x (tde and packaging) on debian/stretch 
> and made a new clean rebuild from scratch (not only tdelibs but all packages 
> required). No building issues whatsoever.

> Someone else might try to reproduce. Are you sure the other patches are in. In 
> my original patch I skipped the test target. I was hoping Fat-Zer will do some 
> work as promised earlier, so that we may work in structured way.

Hi Emanoil, as I said I built from the latest sources and no problems.

What I was thinking (since vcardparser is something you have done some work on the past months) is that you may be using local patches not yet pushed to GIT and those causes the FTBFS.

Could you help to verify this as follow:

1) pull latest R14.1 sources and packaging files
2) make a clean build, without any additional local patch you may be using
3) check whether FTBFS happens or not and let us know.
4) if there was no FTBFS in 3), put back your local patch step by step and identify which patch causes the FTBFS. Then the patch attached to this bug report should go together with the local patch that created the problem

Thanks
Comment 5 deloptes 2016-09-25 02:49:47 CDT
Hi,
I did and indeed it builds. In the build log I do not see executing the test target.

bash test.sh
Checking: ./tests/vcard1.vcf   OK
Checking: ./tests/vcard2.vcf   OK
Checking: ./tests/vcard3.vcf   OK
Checking: ./tests/vcard4.vcf   OK
Checking: ./tests/vcard6.vcf   OK
Checking: ./tests/vcard7.vcf   OK
Checking: ./tests/vcard8.vcf   OK
Checking: ./tests/vcard9.vcf   OK

With the last patch test target is executed.

regards
Comment 6 Michele Calgaro 2016-09-25 03:36:41 CDT
ah, you meant it failed to execute the testing, not that it FTBFS. Perhaps I misunderstood. 
I will test the patch and if good I will commit with your name. How about the CLA? Can I sign off the commit with your name?
Comment 7 Michele Calgaro 2016-09-25 04:03:23 CDT
Patch is good and ready to be pushed. I have just removed those hunks which did not add any functionality, just empty white spaces at the end of the lines.

As soon as you feedback about the CLA (or just confirm that the patch is the result of your own work [which I have no doubt at all] ), I will commit it.

Thanks for the good work!
Comment 8 deloptes 2016-09-25 07:46:25 CDT
Yes Michele. I had a look also I submitted originally without enabling the test target. After I enabled the test target per default it did not execute, because the dependencies were wrongly set.

You could change the target name from test to something else - perhaps testvcards, to skip the CMP0037 warning.

regards
Comment 9 Michele Calgaro 2016-09-25 09:32:05 CDT
> As soon as you feedback about the CLA (or just confirm that the patch is the 
> result of your own work [which I have no doubt at all] ), I will commit it.

What I meant is: what is the status of your CLA? See the "individual contributor" one.
https://www.trinitydesktop.org/cla/

Otherwise you just need to confirm (here is ok) the patch is the result of your own work (which I am sure it is :-) ), then I can commit the patch in your name
Comment 10 deloptes 2016-09-25 10:00:48 CDT
Hi thank you for explanation.
I have not signed CLA (yet). I confirm here that the patch is a result of my own work.
I will study the CLA an come back later.
Comment 11 Michele Calgaro 2016-09-26 07:27:19 CDT
Pushed in commit 13e0329 (R14.1) and 83681c9 (R14.0).
Thanks for the great work Emanoil!
Comment 12 Slávek Banko 2016-09-26 08:11:10 CDT
There is a problem with patch:

/usr/bin/make -f tdeabc/vcardparser/CMakeFiles/test.dir/build.make tdeabc/vcardparser/CMakeFiles/test.dir/build
make[3]: Entering directory `/build/buildd/tdelibs-trinity-14.1.0~r1374/obj-arm-linux-gnueabi'
make[3]: *** No rule to make target `testing', needed by `tdeabc/vcardparser/CMakeFiles/test'.  Stop.
make[3]: Leaving directory `/build/buildd/tdelibs-trinity-14.1.0~r1374/obj-arm-linux-gnueabi'
make[2]: *** [tdeabc/vcardparser/CMakeFiles/test.dir/all] Error 2

See build
https://quickbuild.pearsoncomputing.net/~trinity/+archive/ubuntu/trinity-nightly-builds/+build/333734
Comment 13 Michele Calgaro 2016-09-26 09:18:12 CDT
Strange, it built ok yesterday when I tested.
I will test again tomorrow, today no more time (need to sleep)
Comment 14 Slávek Banko 2016-09-26 11:29:30 CDT
(In reply to Michele Calgaro from comment #13)
> Strange, it built ok yesterday when I tested.
> I will test again tomorrow, today no more time (need to sleep)

It seems that the problem had manifested itself only on Debian 6 (Squeeze) and Ubuntu 10.04 (Lucid). I'll try to look at it later.
Comment 15 deloptes 2016-09-26 12:21:55 CDT
you have in the log
[100%] Built target testing

and next

Scanning dependencies of target test
make[3]: Leaving directory `/build/buildd/tdelibs-trinity-14.1.0~r1374/obj-arm-linux-gnueabi'
/usr/bin/make -f tdeabc/vcardparser/CMakeFiles/test.dir/build.make tdeabc/vcardparser/CMakeFiles/test.dir/build
make[3]: Entering directory `/build/buildd/tdelibs-trinity-14.1.0~r1374/obj-arm-linux-gnueabi'
make[3]: *** No rule to make target `testing', needed by `tdeabc/vcardparser/CMakeFiles/test'.  Stop.

I hope it helps
Comment 16 Slávek Banko 2016-09-26 17:58:53 CDT
Created attachment 2704 [details]
Fix FTBFS on VCardParser tests

If I remember correctly, the names of targets in CMake should be unique in project - hence the names 'test' and 'testing' look very inappropriately. Additionally, creating a test environment does not need to be as a 'target', but may be as a single command. Because creating a test environment in a single command does not need another dependencies, is not called repeatedly == no need to test if symlinks already exists.

In the attached patch has changed the name of the target and simplified commands for creating a test environment. Tested on Stretch and Squeeze => both fine. It remains awkward that the test itself is carried out twice. However, it was also with the prior patch.
Comment 17 Slávek Banko 2016-09-26 18:02:32 CDT
One more note: test.sh looks like it does not contain bashism's == instead of 'bash' could be called 'sh'.
Comment 18 deloptes 2016-09-27 01:46:06 CDT
Thank you Slavek.
I agree. You can read the discussion on the mailing list. There needs to be a test infrastructure throughout the CMake files. I am not that familiar with CMake on how to do it, but it should be consistent through all packages. Fat-Zer mentioned something about changing the packaging files.

Regarding the test.sh ... it shouldn't be there, but I had no idea how to write this in the cmake definition. The second reason was that tests were not working properly and I had to bypass them. I think I know now better.

I suggest we do it right at this level and take care of the global testing later.
Comment 19 Michele Calgaro 2016-09-27 04:18:25 CDT
Created attachment 2707 [details]
new patch

Slavek's patch looks good. I also tested the same patch replacing "bash" with "sh" and the result was good.
I have attached the modified patch.
Comment 20 Slávek Banko 2016-09-27 07:43:52 CDT
Created attachment 2708 [details]
Fundamentally new patch :)

Script test.sh is gone.
Preparation of the test environment is gone.
...everything is cleanly directly in CMake :)

Please, test also on your builder.
Comment 21 Michele Calgaro 2016-09-27 08:39:43 CDT
Also the new patch looks good.
Comment 22 Slávek Banko 2016-09-27 10:50:50 CDT
Pushed to GIT in hash 1571e617 (master) and aca39581 (r14.0.x)
Comment 23 deloptes 2016-09-27 13:27:57 CDT
yes indeed, thank you Slavek and you Michele as well