You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "tkobayas (via GitHub)" <gi...@apache.org> on 2024/03/19 06:05:41 UTC
[PR] [incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… [incubator-kie-drools]
tkobayas opened a new pull request, #5794:
URL: https://github.com/apache/incubator-kie-drools/pull/5794
…ltiKieBaseTest
**Issue**:
* https://github.com/apache/incubator-kie-drools/issues/5709
<details>
<summary>
How to replicate CI configuration locally?
</summary>
Build Chain tool does "simple" maven build(s), the builds are just Maven commands, but because the repositories relates and depends on each other and any change in API or class method could affect several of those repositories there is a need to use [build-chain tool](https://github.com/kiegroup/github-action-build-chain) to handle cross repository builds and be sure that we always use latest version of the code for each repository.
[build-chain tool](https://github.com/kiegroup/github-action-build-chain) is a build tool which can be used on command line locally or in Github Actions workflow(s), in case you need to change multiple repositories and send multiple dependent pull requests related with a change you can easily reproduce the same build by executing it on Github hosted environment or locally in your development environment. See [local execution](https://github.com/kiegroup/github-action-build-chain#local-execution) details to get more information about it.
</details>
<details>
<summary>
How to retest this PR or trigger a specific build:
</summary>
- for <b>pull request and downstream checks</b>
- Push a new commit to the PR. An empty commit would be enough.
- for a <b>full downstream build</b>
- for <b>github actions</b> job: add the label `run_fdb`
- for <b>Jenkins PR check only</b>
- If you are an ASF committer for KIE podling, login to Jenkins (https://ci-builds.apache.org/job/KIE/job/drools/), go to the specific PR job, and click on `Build Now` button.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on PR #5794:
URL: https://github.com/apache/incubator-kie-drools/pull/5794#issuecomment-2006530883
@mariofusco @baldimir @yesamer
Hi folks. This PR, accidentally, uncover two kinds of issues we are currently ignoring:
1. technical debt, that instead of being addressed is workarounded
2. increase of technical debt, polluting the code of such "workarounds"
IMO we have to to change the approach and the mindset, striving to improve the code whenever we have the chance: wdyt ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on code in PR #5794:
URL: https://github.com/apache/incubator-kie-drools/pull/5794#discussion_r1529778339
##########
drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/antlr4/DRLVisitorImpl.java:
##########
@@ -268,6 +268,7 @@ public RuleDescr visitRuledef(DRLParser.RuledefContext ctx) {
ruleDescr.setConsequence(trimThen(getTokenTextPreservingWhitespace(ctx.rhs(), tokenStream))); // RHS is just a text
}
+ populateStartEnd(ruleDescr, ctx);
Review Comment:
The root cause was that `RuleDescr.getStartCharacter()` returned `-1` (missed in `DRLVisitorImpl`), so the kbase was not updated (the error was logged). So fixed.
https://github.com/apache/incubator-kie-drools/blob/dev-new-parser/drools-compiler/src/main/java/org/drools/compiler/kie/util/ChangeSetBuilder.java#L247
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on PR #5794:
URL: https://github.com/apache/incubator-kie-drools/pull/5794#issuecomment-2005849320
Fixed `org.drools.model.codegen.execmodel.MultiKieBaseTest#testHelloWorldWithPackagesAnd2KieBases`. Now all tests in `MultiKieBaseTest` are successful.
Before PR
`drools-model-codegen`
```
[ERROR] Tests run: 2433, Failures: 417, Errors: 2, Skipped: 9
```
After PR
`drools-model-codegen`
```
[ERROR] Tests run: 2433, Failures: 335, Errors: 0, Skipped: 9
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on PR #5794:
URL: https://github.com/apache/incubator-kie-drools/pull/5794#issuecomment-2011626751
@mariofusco @tkobayas @baldimir @yesamer
Follow up [ticket](https://github.com/apache/incubator-kie-issues/issues/1026)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on PR #5794:
URL: https://github.com/apache/incubator-kie-drools/pull/5794#issuecomment-2006830366
@tkobayas
if
> startCharacter and endCharacter are Descr's properties
why they are not already populated at that specific point, and the proposed modification has to populate them ?
>
populating them is not a workaround, rather a right thing to do from the parser point of view.
Yes, I understood that "from the parser point of view" this could be the right fix, but IIUC the problem is somewhere else (please correct me if I'm wrong, I'm simply inferring from your own [comment](https://github.com/apache/incubator-kie-drools/pull/5794#discussion_r1529778339)) and we, as communtiy, have to manage all the codebase as a whole.
As usual, I may be wrong on all my remarks, and I can't wait to talk diorectly with you, to get to a better understanding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on PR #5794:
URL: https://github.com/apache/incubator-kie-drools/pull/5794#issuecomment-2006669771
@gitgabrio Thank you for the suggestion.
`startCharacter` and `endCharacter` are Descr's properties, so populating them is not a workaround, rather a right thing to do from the parser point of view.
Having said that,
- This PR is okay as-is
and
- It's great to refactor/improve the `ChangeSetBuilder.diffDescrs` method, so could you file it as a separate issue?
Does it make sense?
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on PR #5794:
URL: https://github.com/apache/incubator-kie-drools/pull/5794#issuecomment-2011613548
@gitgabrio Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on PR #5794:
URL: https://github.com/apache/incubator-kie-drools/pull/5794#issuecomment-2011136567
> if
>
> > startCharacter and endCharacter are Descr's properties
>
> why they are not already populated at that specific point, and the proposed modification has to populate them ?
`startCharacter` and `endCharacter` should be populated when the RuleDescr is created, and the PR fix does it --- in the same method `DRLVisitorImpl.visitRuledef`.
Talking about the root cause "why we had this bug", I think of
- We can improve the way to create Descr objects not to miss populating the properties in `DRLVisitorImpl`
- Add more test cases
In this regard, I filed https://github.com/apache/incubator-kie-drools/issues/5798
> but IIUC the problem is somewhere else (please correct me if I'm wrong, I'm simply inferring from your own comment)
My comment:
> The root cause was that RuleDescr.getStartCharacter() returned -1 (missed in DRLVisitorImpl), so the kbase was not updated (the error was logged). So fixed.
meant that the `RuleDescr.startCharacter` was `-1` at the point. It had to be populated by `DRLVisitorImpl` beforehand. So I wanted to explain that the root cause was `DRLVisitorImpl`, not `ChangeSetBuilder.diffDescrs`.
> I can't wait to talk directly with you, to get to a better understanding.
Sure, we can talk on meet or zoom.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on PR #5794:
URL: https://github.com/apache/incubator-kie-drools/pull/5794#issuecomment-2005854204
@yurloc @mariofusco @gitgabrio Please review, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #5794:
URL: https://github.com/apache/incubator-kie-drools/pull/5794#discussion_r1529911674
##########
drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/antlr4/DRLVisitorImpl.java:
##########
@@ -268,6 +268,7 @@ public RuleDescr visitRuledef(DRLParser.RuledefContext ctx) {
ruleDescr.setConsequence(trimThen(getTokenTextPreservingWhitespace(ctx.rhs(), tokenStream))); // RHS is just a text
}
+ populateStartEnd(ruleDescr, ctx);
Review Comment:
Hi @tkobayas , thanks for the PR!
IIUC the comment, it means that a [piece of code](https://github.com/apache/incubator-kie-drools/blob/dev-new-parser/drools-compiler/src/main/java/org/drools/compiler/kie/util/ChangeSetBuilder.java#L247) does not populate correctly a given object (`ruleDescr`), and your modification somehow "workaround" that.
I read the [method](https://github.com/apache/incubator-kie-drools/blob/5f83f08f169c0c4922e7bd842ae612c607f96dc8/drools-compiler/src/main/java/org/drools/compiler/kie/util/ChangeSetBuilder.java#L223), and TBH is somehow "brittle" by itself (I may give detailed explanation of this remark, if you want): would not be better to fix the root problem itself ? Eventually, I would be glad to help there
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas merged PR #5794:
URL: https://github.com/apache/incubator-kie-drools/pull/5794
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… [incubator-kie-drools]
Posted by "yurloc (via GitHub)" <gi...@apache.org>.
yurloc commented on PR #5794:
URL: https://github.com/apache/incubator-kie-drools/pull/5794#issuecomment-2012133796
Overall fixing progress: There were 407 failed tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org