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.

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

Don't filter gradle's stderr anymore #860

Merged
merged 1 commit into from
Jan 6, 2020

Conversation

raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Nov 3, 2019

Motivation and Context

When building the project by running gradlew, we filter its output on stderr to omit a specific diagnostic message. This complicated refactorings in the past and also affects upcoming refactorings regarding Q and superspawn.

The reason this code exists is a weak one IMHO. The Visual Studio integration deemed the build to have failed if anything has been output on stderr. Thus the aforementioned diagnostic message on stderr caused successful builds to count as failed ones.

IMHO, the current behavior should be removed for various reasons:

  • Diagnostics on stderr are not uncommon, so the success of the command should be determined from the returned Promise or the process' exit code respectively
  • Unclear if code works correctly & reliably: the code is written as if only complete lines were passed into progress. But superspawn calls notify for every chunk it receives in a data event on stderr. AFAIK it is not guaranteed that these chunks will be individual and complete lines.
  • Unclear if code is up-to-date
  • Code is untested
  • Complicates refactorings

Description

This PR removes all output filtering in ProjectBuilder.prototype.build.

Testing

Automated tests still pass.

@raphinesse raphinesse added this to the 9.0.0 milestone Nov 3, 2019
@codecov-io
Copy link

codecov-io commented Nov 3, 2019

Codecov Report

Merging #860 into master will increase coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #860      +/-   ##
==========================================
+ Coverage   66.25%   66.46%   +0.21%     
==========================================
  Files          19       19              
  Lines        1861     1855       -6     
==========================================
  Hits         1233     1233              
+ Misses        628      622       -6
Impacted Files Coverage Δ
...n/templates/cordova/lib/builders/ProjectBuilder.js 67.79% <100%> (+2.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcaab36...e66f664. Read the comment docs.

Copy link
Member

@timbru31 timbru31 left a comment

Choose a reason for hiding this comment

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

LGTM - it would be great if this could be tested though. Still +1, given the fact that Microsoft seems to decide to ditch .jsproj (and maybe more) with the recent Visual Studio update.

@erisu erisu merged commit e3cc75c into apache:master Jan 6, 2020
@raphinesse raphinesse deleted the drop-gradle-build-stderr-filter branch January 7, 2020 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants