feat(spdx2): SPDX output does not yet show license comments #1267

ChristopheRequillart · 2019-01-10T11:26:52Z

Description

Implement SPDX output does not yet show license comments

Changes

add a new column in spdx_license_comment in upload table.
add a new option in Organize/Uploads/Edit Properties to enable or disable this feature.
add the LicenseComment field for both SPDX: Tag-Value and SPDX: RDF exports only if the option is activated.

How to test

Activate the new option in Organize/Uploads/Edit Properties and launch an export SPDX.

closing #645

GMishx · 2019-01-23T10:06:01Z

Hello @ChristopheRequillart, have you consider fixing the test cases?

ChristopheRequillart · 2019-01-23T10:18:33Z

Hello @GMishx
Which test cases? I don’t change any test case in my patch.

GMishx · 2019-01-23T10:20:30Z

If you check the Travis logs here: https://travis-ci.org/fossology/fossology/builds/477779703?utm_source=github_status&utm_medium=notification
You can find the failing test cases which needs to be fixed.

ChristopheRequillart · 2019-01-23T11:39:49Z

I see the Travis logs. As I said I don’t touch Test files in my patch. I search in this code and I found only that pclose return -1. Has Travis a problem?
see ./src/decider/agent_tests/Functional/SchedulerTestRunnerCli.php line 64
$retCode = pclose($pipeFd);

GMishx · 2019-01-23T12:17:19Z

The Travis is fine, the changes you made are not reflected in the test cases for the agent.
Head over to fossology/src/spdx2/agent_tests/ to get the test cases under Unit and Functional folders and make changes according to the changes made in agent.

You can check similar case here: https://github.com/fossology/fossology/pull/1272/files#diff-d3783e41ad5557d508cc197dffb9dcee

ChristopheRequillart · 2019-01-23T14:24:31Z

I do an error on the filename on my previous post. It’s ./src/spdx2/agent_tests/Functional/SchedulerTestRunnerCli.php and not ./src/decider/agent_tests/Functional/SchedulerTestRunnerCli.php. But the problem is the same.
In ./src/spdx2/agent_tests/Functional/schedulerTest.php you can see that error reported in Travis log is line 190:
assertThat('report failed: "'.$output.'"', $retCode, equalTo(0));

$retCode is initialized line 188 same file:
list($success,$output,$retCode) = $this->runnerCli->run($uploadId, $this->userId, $this->groupId, $jobId);

$this->runnerCli is aSchedulerTestRunnerCli object. On line 62 of file ./src/spdx2/agent_tests/Functional/SchedulerTestRunnerCli.php in aSchedulerTestRunnerCli::run() you can see that retCode is the return of pclose.
Text error reported in Travis logs is:

VERSION: 3.4.0-34-ga67e9ee
OK

Why are you sure it is a problem of changes made in spdx agent?

mcjaeger · 2019-02-07T09:03:15Z

(from discussion) @ChristopheRequillart actually, we are not sure.

mcjaeger · 2019-02-07T10:50:09Z

Tested, works in general, but not with two licenses and their comments. @ChristopheRequillart Would it be Ok, to change it a little to concatenate both strings maybe in a <text>Adaptec COMMENT 1</text>, text>GPL COMMENT 2</text>?

Example currently:

-> only one comment

coming from:

mcjaeger · 2019-02-07T10:53:21Z

adding to it: If there are multiple licenses, but only one of these has a comment, this one comment will be printed in any case (regardless it is a comment of the first lic in the list or the last) -> OK.

ChristopheRequillart · 2019-02-07T15:54:19Z

I have a patch using SpdxTwoUtils::implodeLicenses() but it is <text>commentaire AND test commentaire 2 </text>. Are you ok with this?

GMishx · 2019-02-08T13:01:38Z

Kindly rebase with origin master (git pull --rebase origin master).

mcjaeger · 2019-03-11T08:47:58Z

@ChristopheRequillart , just a minor thing:
feat(scm): patch for two or more licenses comments
-> should be feat(SPDX2): ...?
would you mind to squash the commit anyway?

mcjaeger · 2019-03-13T12:55:06Z

hm, it seems like there is the same error for the tests ran:

Time: 2.24 seconds, Memory: 10.00MB
There were 2 errors:
1) Fossology\SpdxTwo\Test\SchedulerTest::testSpdxForNormalUploadtreeTable
Hamcrest\AssertionError: report failed: "VERSION: 3.4.0-95-g4d86913
OK
"
Expected: <0>
     but: was <255>
/home/travis/build/fossology/fossology/src/vendor/hamcrest/hamcrest-php/hamcrest/Hamcrest/MatcherAssert.php:115
/home/travis/build/fossology/fossology/src/vendor/hamcrest/hamcrest-php/hamcrest/Hamcrest/MatcherAssert.php:63
/home/travis/build/fossology/fossology/src/vendor/hamcrest/hamcrest-php/hamcrest/Hamcrest.php:28
/home/travis/build/fossology/fossology/src/spdx2/agent_tests/Functional/schedulerTest.php:190
/home/travis/build/fossology/fossology/src/spdx2/agent_tests/Functional/schedulerTest.php:226
/home/travis/build/fossology/fossology/src/spdx2/agent_tests/Functional/schedulerTest.php:160
2) Fossology\SpdxTwo\Test\SchedulerTest::testSpdxForSpecialUploadtreeTable
Hamcrest\AssertionError: report failed: "VERSION: 3.4.0-95-g4d86913
OK
"
Expected: <0>
     but: was <255>
/home/travis/build/fossology/fossology/src/vendor/hamcrest/hamcrest-php/hamcrest/Hamcrest/MatcherAssert.php:115
/home/travis/build/fossology/fossology/src/vendor/hamcrest/hamcrest-php/hamcrest/Hamcrest/MatcherAssert.php:63
/home/travis/build/fossology/fossology/src/vendor/hamcrest/hamcrest-php/hamcrest/Hamcrest.php:28
/home/travis/build/fossology/fossology/src/spdx2/agent_tests/Functional/schedulerTest.php:190
/home/travis/build/fossology/fossology/src/spdx2/agent_tests/Functional/schedulerTest.php:226
/home/travis/build/fossology/fossology/src/spdx2/agent_tests/Functional/schedulerTest.php:178
ERRORS!
Tests: 2, Assertions: 0, Errors: 2.
make[4]: *** [test-sched] Error 2
make[4]: Leaving directory `/home/travis/build/fossology/fossology/src/spdx2/agent_tests/Functional'
make[3]: *** [test] Error 2
make[3]: Leaving directory `/home/travis/build/fossology/fossology/src/spdx2/agent_tests'
make[2]: *** [test] Error 2
make[2]: Leaving directory `/home/travis/build/fossology/fossology/src/spdx2'
make[1]: *** [test-spdx2] Error 2
make[1]: Leaving directory `/home/travis/build/fossology/fossology/src'
make: *** [test-src] Error 2
The command "make test" exited with 2.
Done. Your build exited with 1.

ChristopheRequillart · 2019-03-13T15:00:41Z

Hello Michael,
In src/spdx2/agent_tests/Functional/SchedulerTestRunnerCli.php in run function, we can see that retCode is the return of pclose($pipeFd). $pipeFd is the output of open($cmd = "echo $uploadId | $execDir/$agentName --userID=$userId ". "--groupID=$groupId --jobId=$jobId --scheduler_start -c $sysConf $args", "r");.
The test is done in ./src/spdx2/agent_tests/Functional/schedulerTest.php on line 190:
assertThat('report failed: "'.$output.'"', $retCode, equalTo(0));

But I can’t get it when I run it.

# echo 47 | /usr/share/fossology/spdx2/agent/spdx2.sh --userID=3 --groupID=3 --jobId=2 --scheduler_start -c /etc/fossology/
VERSION: 3.4.0-94-gaa242c7
OK
HEART: 0 1
BYE 0
# echo $?
0

ChristopheRequillart · 2019-03-13T16:19:35Z

But if I put an uploadId without data in uploadtree_a, I get it:

# echo 93 | /usr/share/fossology/spdx2/agent/spdx2 --userID=3 --groupID=3 --jobId=2 --scheduler_start -c /etc/fossology/
VERSION: 3.4.0-94-gaa242c7
OK
# echo $?
255

Is there a problem with $uploadId when Travis launch the test?

ChristopheRequillart · 2019-03-19T10:51:50Z

Doesn’t seem the same problem when I call with an uploadId without data in uploadtree_a:

ar 19 11:44:27 fossyserver php[3538]: PHP Fatal error:  Uncaught TypeError: Argument 1 passed to Fossology\SpdxTwo\SpdxTwoAgent::getFilesWithLicensesFromClearings() must be an instance of Fossology\Lib\Data\Tree\ItemTreeBounds, bool given, called in /usr/share/fossology/spdx2/agent/spdx2.php on line 326 and defined in /usr/share/fossology/spdx2/agent/spdx2.php:373
Mar 19 11:44:27 fossyserver php[3538]: Stack trace:
Mar 19 11:44:27 fossyserver php[3538]: #0 /usr/share/fossology/spdx2/agent/spdx2.php(326): Fossology\SpdxTwo\SpdxTwoAgent->getFilesWithLicensesFromClearings(false)
Mar 19 11:44:27 fossyserver php[3538]: #1 /usr/share/fossology/spdx2/agent/spdx2.php(221): Fossology\SpdxTwo\SpdxTwoAgent->renderPackage(93)
Mar 19 11:44:27 fossyserver php[3538]: #2 /usr/share/fossology/lib/php/Agent/Agent.php(341): Fossology\SpdxTwo\SpdxTwoAgent->processUploadId(93)
Mar 19 11:44:27 fossyserver php[3538]: #3 /usr/share/fossology/spdx2/agent/spdx2.php(874): Fossology\Lib\Agent\Agent->run_scheduler_event_loop()
Mar 19 11:44:27 fossyserver php[3538]: #4 /usr/lib/fossology/fo_wrapper(77): require('/usr/share/foss...')
Mar 19 11:44:27 fossyserver php[3538]: #5 {main}
Mar 19 11:44:27 fossyserver php[3538]:   thrown in /usr/share/fossology/spdx2/agent/spdx2.php on line 373

src/spdx2/agent/spdx2.php

GMishx · 2019-04-09T10:44:26Z

The error on SPDX test appears to be an issue with PHP itself. I tried to run it on Trusty Docker image with PHP 5.6 and it failed but with PHP 7.0 from ondrej PPA passed on the same container.

It might get fixed after #1325

ChristopheRequillart · 2019-04-09T12:26:16Z

@GMishx
I don’t know because in beginning of Travis log, you can find:
$ php --version PHP 7.0.25 (cli) (built: Oct 26 2017 16:27:50) ( ZTS )

maxhbr

minor comments

src/spdx2/agent/spdx2.php

maxhbr · 2019-04-09T16:55:10Z

src/spdx2/agent/template/spdx2tv-file.twig

+LicenseComment: NOCOMMENT
+{% else %}
+LicenseComments: <text>{{ licenseComment |replace({'<text>':'&lt;text&gt;','</text>':'&lt;/text&gt;'})
+                                         |replace({'\f':''}) }} </text>


here is |e missing, as used in https://github.com/fossology/fossology/pull/1267/files#diff-11fb262498a75525792bb019f78fe519R45

Here LicenseComments follow the same logic as FileCopyrightText, just below. Why don’t keep this homogeneous?

src/spdx2/agent/template/spdx2tv-file.twig

ChristopheRequillart · 2019-04-10T17:14:05Z

Hello, Nicolas give me an idea to be more compliant with php 5.6. So I delete type declaration for parameter of methods. With this, some tests of Travis passed OK. Thanks Nicolas. I don’t understand the last error in Travis test.

mcjaeger · 2019-05-02T11:37:36Z

@ChristopheRequillart please consider the merge conflicts - every day makes it worse ...

ChristopheRequillart · 2019-05-02T12:10:59Z

Thanks Michael, merge done.

maxhbr · 2019-05-02T12:15:54Z

@ChristopheRequillart we usually solve conflicts by rebasing and then force pushing to the branch. This yields a simpler history.

…name array in generateFileNodesByFiles() and create array correctly in getSPDXLicenseCommentState()

Co-Authored-By: ChristopheRequillart <[email protected]>

ChristopheRequillart · 2019-05-02T12:35:19Z

OK rebase & forced push done

ChristopheRequillart · 2019-05-13T16:42:29Z

Hello,
Why a change request is always active?
All issues are resolved for me.

mcjaeger · 2019-05-21T10:36:22Z

@ChristopheRequillart we have merge #1349 while trying to preserve your commit history. We thought it might be easier just to make some modifications.

ghost added the needs code review label Jan 10, 2019

mcjaeger self-assigned this Feb 7, 2019

mcjaeger added this to the 3.5.0 milestone Feb 7, 2019

mcjaeger removed their assignment Feb 7, 2019

ChristopheRequillart force-pushed the ChristopheRequillart/645/SPDX-output-does-not-yet-show-license-comments branch 2 times, most recently from 3e65d0e to 2f83b8e Compare February 8, 2019 14:41

ChristopheRequillart force-pushed the ChristopheRequillart/645/SPDX-output-does-not-yet-show-license-comments branch from 2f83b8e to aa242c7 Compare March 11, 2019 09:33

mcjaeger modified the milestones: 3.5.0, 3.6.0 Apr 1, 2019

GMishx requested changes Apr 9, 2019

View reviewed changes

src/spdx2/agent/spdx2.php Outdated Show resolved Hide resolved

src/spdx2/agent/spdx2.php Outdated Show resolved Hide resolved

ChristopheRequillart force-pushed the ChristopheRequillart/645/SPDX-output-does-not-yet-show-license-comments branch from aa242c7 to 0204ea6 Compare April 9, 2019 12:40

ghost assigned ChristopheRequillart Apr 9, 2019

maxhbr reviewed Apr 9, 2019

View reviewed changes

NicolasToussaint reviewed Apr 10, 2019

View reviewed changes

src/spdx2/agent/template/spdx2tv-file.twig Outdated Show resolved Hide resolved

ChristopheRequillart force-pushed the ChristopheRequillart/645/SPDX-output-does-not-yet-show-license-comments branch from a5dc1b2 to aca09d6 Compare April 10, 2019 16:15

GMishx added the has merge conflicts PR to be rebased label Apr 23, 2019

ChristopheRequillart force-pushed the ChristopheRequillart/645/SPDX-output-does-not-yet-show-license-comments branch from ae78945 to 610bc36 Compare May 2, 2019 12:30

ChristopheRequillart and others added 6 commits May 2, 2019 14:33

feat(spdx2): SPDX output does not yet show license comments

b6ada97

feat(spdx2): patch for two or more licenses comments

3ef620d

(spdx2): avoid two calls to database when searching comment state, re…

6290adc

…name array in generateFileNodesByFiles() and create array correctly in getSPDXLicenseCommentState()

(spdx2) Simplify array creation

53e4d72

Co-Authored-By: ChristopheRequillart <[email protected]>

(spdx2) Don’t use NOCOMMENT

c5cd7e1

(spdx2) don’t use type declaration to be compliant php 5.6

38072e8

ChristopheRequillart force-pushed the ChristopheRequillart/645/SPDX-output-does-not-yet-show-license-comments branch from 610bc36 to 38072e8 Compare May 2, 2019 12:33

feat(spdx2): correct some typos after Travis tests

96ee541

mcjaeger removed the has merge conflicts PR to be rebased label May 2, 2019

mcjaeger closed this May 21, 2019

feat(spdx2): SPDX output does not yet show license comments #1267

feat(spdx2): SPDX output does not yet show license comments #1267

Uh oh!

Conversation

ChristopheRequillart commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

How to test

Uh oh!

GMishx commented Jan 23, 2019

Uh oh!

ChristopheRequillart commented Jan 23, 2019

Uh oh!

GMishx commented Jan 23, 2019

Uh oh!

ChristopheRequillart commented Jan 23, 2019

Uh oh!

GMishx commented Jan 23, 2019

Uh oh!

ChristopheRequillart commented Jan 23, 2019

Uh oh!

mcjaeger commented Feb 7, 2019

Uh oh!

mcjaeger commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcjaeger commented Feb 7, 2019

Uh oh!

ChristopheRequillart commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GMishx commented Feb 8, 2019

Uh oh!

mcjaeger commented Mar 11, 2019

Uh oh!

mcjaeger commented Mar 13, 2019

Uh oh!

ChristopheRequillart commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChristopheRequillart commented Mar 13, 2019

Uh oh!

ChristopheRequillart commented Mar 19, 2019

Uh oh!

Uh oh!

Uh oh!

GMishx commented Apr 9, 2019

Uh oh!

ChristopheRequillart commented Apr 9, 2019

Uh oh!

maxhbr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maxhbr Apr 9, 2019

Choose a reason for hiding this comment

Uh oh!

ChristopheRequillart Apr 10, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChristopheRequillart commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcjaeger commented May 2, 2019

Uh oh!

ChristopheRequillart commented May 2, 2019

Uh oh!

maxhbr commented May 2, 2019

Uh oh!

ChristopheRequillart commented May 2, 2019

Uh oh!

ChristopheRequillart commented May 13, 2019

Uh oh!

mcjaeger commented May 21, 2019

Uh oh!

Reviewers

Assignees

Labels

ChristopheRequillart commented Jan 10, 2019 •

edited

Loading

mcjaeger commented Feb 7, 2019 •

edited

Loading

ChristopheRequillart commented Feb 7, 2019 •

edited

Loading

ChristopheRequillart commented Mar 13, 2019 •

edited

Loading

ChristopheRequillart commented Apr 10, 2019 •

edited

Loading