Update JavaCompileUsingEcj.kt#1518
Conversation
Builds on Windows assuming there is a C++ compiler
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1518 +/- ##
============================================
- Coverage 50.21% 50.20% -0.01%
Complexity 12650 12650
============================================
Files 1365 1365
Lines 85214 85214
Branches 14733 14733
============================================
- Hits 42787 42784 -3
- Misses 37632 37633 +1
- Partials 4795 4797 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
liblit
left a comment
liblit
left a comment
There was a problem hiding this comment.
Thanks for proposing these changes! I have a few refinements to suggest, most of which involve making your improvements more Kotlin-idiomatic.
| jdtPrefs.toString(), | ||
| "-classpath", | ||
| [email protected](":"), | ||
| [email protected](File.pathSeparator), |
There was a problem hiding this comment.
Attention @msridhar: this pull request implies that ECJ builds on Windows were not previously working, but I think we didn't know that. Should we be enabling ECJ builds on Windows so that we will be aware of any future regressions to that capability? Should we enable ECJ builds on macOS too?
Or conversely, do we neither care about nor support ECJ builds on Windows, in which case this whole pull request should be discarded?
There was a problem hiding this comment.
We actually need Windows builds here at IBM, since we have product usage of WALA on Windows (not mine!). So please can we keep support for building it all on Windows?
There was a problem hiding this comment.
Sure, I think we should probably just enable ECJ on all builds on CI. It shouldn't cost us much CI time compared to running tests.
There was a problem hiding this comment.
OK, we're all agreed: CI will include ECJ builds on all supported platforms.
@juliandolby: it's OK with me if you want to leave this alone for now. Once the current pull request is merged, I'll create a separate pull request to enable ECJ builds everywhere.
build-logic/src/main/kotlin/com/ibm/wala/gradle/JavaCompileUsingEcj.kt
Outdated
Show resolved
Hide resolved
build-logic/src/main/kotlin/com/ibm/wala/gradle/JavaCompileUsingEcj.kt
Outdated
Show resolved
Hide resolved
build-logic/src/main/kotlin/com/ibm/wala/gradle/JavaCompileUsingEcj.kt
Outdated
Show resolved
Hide resolved
build-logic/src/main/kotlin/com/ibm/wala/gradle/JavaCompileUsingEcj.kt
Outdated
Show resolved
Hide resolved
build-logic/src/main/kotlin/com/ibm/wala/gradle/JavaCompileUsingEcj.kt
Outdated
Show resolved
Hide resolved
|
have tried to make the changes more idiomatic, but i am not a kotlin programmer... |
liblit
left a comment
liblit
left a comment
There was a problem hiding this comment.
I think I can improve this code further, but let's not let perfect be the enemy of good. I'll approve this PR in its current form. If I think I can make it even nicer, then I'll propose those changes separately.
|
FYI, I just noticed that the ECJ-compiled bytecode files for some |
Previously, Linux jobs ran with an X Virtual Framebuffer (Xvfb). However, we no longer have any tests that require an X display. Previously Linux jobs included ECJ compilation but macOS and Windows jobs did not. [Unfortunately, that allowed a Windows-specific ECJ compilation problem to go undetected.](wala#1518) In discussion of that fix, WALA maintainers agreed that testing ECJ on all platforms was advisable. Previously, there were a few other minor things that we also only tested on Linux. For simplicity and thoroughness, it seems wise to test the same things on all platforms.
Previously Linux jobs included ECJ compilation but macOS and Windows jobs did not. [Unfortunately, that allowed a Windows-specific ECJ compilation problem to go undetected.](wala#1518) In discussion of that fix, WALA maintainers agreed that testing ECJ on all platforms was advisable.
Previously Linux jobs included ECJ compilation but macOS and Windows jobs did not. [Unfortunately, that allowed a Windows-specific ECJ compilation problem to go undetected.](#1518) In discussion of that fix, WALA maintainers agreed that testing ECJ on all platforms was advisable.
4 participants
Builds on Windows assuming there is a C++ compiler
Thank you for contributing to WALA! Please see the contribution guidelines at https://github.com/wala/WALA/blob/master/CONTRIBUTING.md for information on code style and general guidelines for pull requests.