Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed multithreaded gradle #1337

Merged
merged 9 commits into from
Sep 9, 2024
Merged

Fixed multithreaded gradle #1337

merged 9 commits into from
Sep 9, 2024

Conversation

malice00
Copy link
Contributor

This PR fixes #1335 , although the solution for that issue may need to be redone if someone can reproduce the actual use-case -- instead of a non-existent parameter, a hard-coded String is now used.

After fixing that, I found a several of other problems with the multithreaded solution:

  • Arguments specific for a task in the wrong place
  • Arguments specific for a task must be added for every call to the task
  • Unlike in the single runs, gradle fails all jobs if just one task has an issue and NO dependencies are returned

So, the necessary arguments have been multiplied, all arguments have been arranged to their correct places and a new envvar has been introduced to be able to skip problematic modules.

@malice00 malice00 requested a review from prabhu as a code owner August 28, 2024 22:23
@prabhu
Copy link
Collaborator

prabhu commented Aug 29, 2024

@ajmalab could you advise and work with @malice00 to get this ready?

prabhu
prabhu reviewed Sep 1, 2024
View reviewed changes
utils.js Outdated
@@ -2878,40 +2878,37 @@ export function parseGradleProperties(rawOutput) {
*/
export function executeParallelGradleProperties(dir, rootPath, allProjectsStr) {
const defaultProps = {
rootProject: subProject,
rootProject: "root",
Copy link
Collaborator

@prabhu prabhu Sep 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of hardcoding, can we set this to spstr from the for loop in line 2905?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have look, this was just a quick fix/hack to get things working.

@malice00
Copy link
Contributor Author

malice00 commented Sep 1, 2024

I've renamed some of the variables to make the code less confusing. Some variables where still called 'purl' (or some variant thereof) but where actually a 'bom-ref'.
Also rebased to the current master.

prabhu
prabhu reviewed Sep 1, 2024
View reviewed changes
docs/ENV.md
@@ -24,6 +24,7 @@ The following environment variables are available to configure the bom generatio
| GRADLE_HOME | Specify gradle home |
| GRADLE_CMD | Set to override gradle command |
| GRADLE_DEPENDENCY_TASK | By default cdxgen use the task "dependencies" to collect packages. Set to override the task name. |
| GRADLE_SKIP_MODULES | Comma-separated list of modules to skip during the "dependencies" task. This can be useful if you have modules that would fail the gradle build, eg when they do not have dependencies in the given configuration. Use "root" if the top most module should be skipped, use the name (without leading ":") for all others. |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add an example to the repo tests?

@malice00
Copy link
Contributor Author

malice00 commented Sep 1, 2024 •
edited
Loading

I completely removed the object, since this method should return a String -- even according to its documentation.

I assume this was copied from the singlethreaded version of the method, which implements parsing the output as well. Since the multithreaded variant does in fact not parse the output, the defaultProps are not necessary here.

@prabhu
Copy link
Collaborator

prabhu commented Sep 1, 2024

Interesting. Can we add some repo tests to prove that the sbom generated with and without multi threading is identical? We can use this project similar to our snapshot tests.

@malice00
Copy link
Contributor Author

malice00 commented Sep 1, 2024

I'll take a stab at it...

@prabhu
Copy link
Collaborator

prabhu commented Sep 2, 2024

Please let me know once you are done and ready.

@prabhu prabhu marked this pull request as draft September 2, 2024 11:52
@malice00
Copy link
Contributor Author

malice00 commented Sep 2, 2024

Honestly, I'm unsure on how to get a good test in here without setting up an actual (small) project, which would then need Gradle (and therefore Java) and probably some other stuff... Maybe it will be easier to do something like this when you redesign cdxgen in the next major version...
But if anyone has any ideas on how to get a test up and running that can generate the whole SBOM, I'm open to suggestions!

@prabhu
Copy link
Collaborator

prabhu commented Sep 3, 2024

Java and gradle are already setup in repotests, so we only need a sample repo.

@malice00
Copy link
Contributor Author

malice00 commented Sep 3, 2024

Ah, great! I'm not seeing them though -- where are these tests located, so I can check out what we'd need and how to get it running?

@prabhu
Copy link
Collaborator

prabhu commented Sep 3, 2024

https://github.com/CycloneDX/cdxgen/blob/master/.github/workflows/repotests.yml

@malice00
Copy link
Contributor Author

malice00 commented Sep 5, 2024

I added a test, but for some reason it's not doing anything... Even with CDXGEN_DEBUG_MODE I don't see anything -- how do I get more output from cdxgen to figure out what might be wrong?

As for that cjd project, I added it and a call to it, but I see no way how this may help in a test -- it simply generates a report which must manually be checked, or am I missing something?

@prabhu
Copy link
Collaborator

prabhu commented Sep 6, 2024

@aryan-rajoria could you kindly take a look and suggest some repo tests? We can merge this PR and add the tests separately if it might be easier.

@malice00
Copy link
Contributor Author

malice00 commented Sep 6, 2024

Does anybody know why we don't see the output of cdxgen in these repotests? Would make stuff a lot easier to check/debug...
Also, wouldn't it be better if the 'tests' actually had assertions or something like that? Right now they seem to just run a build and only fail when something goes horribly wrong. My tests should eg fail if (like now) the SBOM has no components. And a check like you suggested with cjb would be great, if only that tool could actually fail the build, eg with an exit code if the SBOMs are different...
But maybe this is better in its own issue though. 😄

@prabhu
Copy link
Collaborator

prabhu commented Sep 8, 2024

@malice00 good points raised. We tried coming up with snapshot tests to track the diff and fail the PR. Testing, in general, is going to take some more effort to get there. As part of another PR, I have temporarily hardcoded "root" as the subProject name. Since there is no response from Ajmal, can we park this PR and resume once the refactoring as part of #1360 is complete?

@malice00
Copy link
Contributor Author

malice00 commented Sep 8, 2024

@prabhu, I'm not thrilled about that, because I'm working on some other changes that build on this. Any idea when #1360 will be merged?

@malice00
Fixed multithreaded gradle
60fcf3f 
Signed-off-by: Roland Asmann <roland.asmann@gmail.com>
@malice00
Renamed some variables for better readability of the code
2962cdf 
Signed-off-by: Roland Asmann <roland.asmann@gmail.com>
@malice00
This method is returning a String in non-error case, so error should …
7372787 
…not return an object!

Moved returning the empty String to after logging of information on how to debug errors.

Signed-off-by: Roland Asmann <roland.asmann@gmail.com>
@malice00
Removed a parameter that was ALWAYS null
336fb6a 
Signed-off-by: Roland Asmann <roland.asmann@gmail.com>
@malice00
No need to get properties for the root again
f9534ad 
Signed-off-by: Roland Asmann <roland.asmann@gmail.com>
@malice00
Also parse ignored modules, but use the empty String as input
76e6de5 
Signed-off-by: Roland Asmann <roland.asmann@gmail.com>
@malice00
Added a helper-method to build the list of arguments for all gradle c…
9e0c126 
…alls

Signed-off-by: Roland Asmann <roland.asmann@gmail.com>
@malice00
Added repotest for multithreaded gradle
59112cf 
Signed-off-by: Roland Asmann <roland.asmann@gmail.com>
@malice00
Added DEBUG to figure out what is going on
a0c818e 
Signed-off-by: Roland Asmann <roland.asmann@gmail.com>
@prabhu
Copy link
Collaborator

prabhu commented Sep 8, 2024

@setchy are you happy to get this merged and ported to the refactor branch? Will be great if we all could collaborate since freezing the project for a large scale refactor is proving difficult

@setchy
Copy link
Member

setchy commented Sep 8, 2024

No need to freeze the project - happy to merging ongoing updates into the refactor branch. I don't want that initial refactor to be more than just moving files around and import updates

@malice00 malice00 mentioned this pull request Sep 9, 2024
Merged
@prabhu prabhu marked this pull request as ready for review September 9, 2024 03:59
@prabhu prabhu merged commit e7be44f into CycloneDX:master Sep 9, 2024
18 of 21 checks passed
@malice00 malice00 deleted the fix/gradle_multithread branch September 9, 2024 06:24
@malice00
Copy link
Contributor Author

malice00 commented Sep 9, 2024 •
edited
Loading

@prabhu Awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prabhu prabhu prabhu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Gradle] GRADLE_MULTI_THREADED throws an error
3 participants
@malice00 @prabhu @setchy