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

CB-14108: fix incorrect count in config_munge in ios.json and android.json #24

Merged
merged 10 commits into from Sep 11, 2018

Conversation

@knight9999
Copy link
Contributor

@knight9999 knight9999 commented May 30, 2018

Platforms affected

All Platforms

What does this PR do?

This PR affects the cordova prepare command as follows.

  1. Check the confliction of config-file tag in config.xml.
  2. Prevent unexpected increasing count in config_munge in platformJson (ios/ios.json or android/android.json).
  3. Removing unnecessary config_munge directive in platformJson after removing config-file tag.

Futrher, adding related test codes. (ConfigError.spec.js, ConfigChanges.spec.js)

What testing has been done on this change?

Before this PR, if we use config.file like

   <platform name="ios">
        <allow-intent href="itms:*" />
        <allow-intent href="itms-apps:*" />
        <config-file parent="NSCameraUsageDescription" target="*-Info.plist">
            <string>Please permit Camera!</string>
        </config-file>
    </platform>

, then cordova prepare makes platforms/ios/ios.json

 "config_munge": {
    "files": {
      "*-Info.plist": {
        "parents": {
          "NSCameraUsageDescription": [
            {
              "xml": "<string>Please permit Camera!</string>",
              "count": 1
            }
          ]
        }
      }
    }
  },

note that count is 1. Again doing cordova prepare makes platforms/ios/ios.json

 "config_munge": {
    "files": {
      "*-Info.plist": {
        "parents": {
          "NSCameraUsageDescription": [
            {
              "xml": "<string>Please permit Camera!</string>",
              "count": 2
            }
          ]
        }
      }
    }
  },

where the count is 2.
Further if we remove config-file tag in config.xml, cordova prepare does not remove anything from platforms/ios/ios.json.

This unnatural behavior is fixed by this PR.

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.
@codecov-io
Copy link

@codecov-io codecov-io commented May 30, 2018

Codecov Report

Merging #24 into master will increase coverage by 1.58%.
The diff coverage is 94.05%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #24      +/-   ##
=========================================
+ Coverage   85.51%   87.1%   +1.58%     
=========================================
  Files          19      19              
  Lines        1761    1830      +69     
  Branches      371     384      +13     
=========================================
+ Hits         1506    1594      +88     
+ Misses        255     236      -19
Impacted Files Coverage Δ
src/ConfigParser/ConfigParser.js 76.73% <ø> (ø) ⬆️
src/ConfigChanges/munge-util.js 95.45% <100%> (+3.56%) ⬆️
src/ConfigChanges/ConfigFile.js 91.54% <60%> (+1.69%) ⬆️
src/ConfigChanges/ConfigChanges.js 94.18% <95.12%> (-0.5%) ⬇️
src/util/plist-helpers.js 95.34% <0%> (+13.95%) ⬆️
src/CordovaError/CordovaError.js 89.65% <0%> (+44.82%) ⬆️

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 ff3630f...fe034d9. Read the comment docs.

@dpogue
Copy link
Member

@dpogue dpogue commented May 30, 2018

Thanks for the pull request!

I'm not especially familiar with the config munging code, but I know it has been a source of edge case bugs and particular around multiple plugin.xml/config.xml wanting to make conflicting changes. There are a few open issues in JIRA (CB-13474, CB-13486, and CB-13514 in particular) related to those edge cases, and I wanted to check if you thought your changes will have any impact (positive or negative) on those.

@knight9999
Copy link
Contributor Author

@knight9999 knight9999 commented May 31, 2018

Thanks, @dpogue. I try to check the issues CB-13474, CB-13486 and CB-13514.

@knight9999
Copy link
Contributor Author

@knight9999 knight9999 commented May 31, 2018

Sorry, CB-13474, CB-13486 and CB-13514 are the issues in which multiple config-files and/or edit-configs compete each other. (I.e. xml conflicting)
However my PR does not resolve this kind of conflicting.
This PR is keeping necessary previous config-file settings and removing unnecessary previous config-file settings. Then applying the new config-file settings correctly.

@janpio janpio added this to 🐣 New PR / Untriaged in Apache Cordova: Tooling Pull Requests Sep 5, 2018
@janpio
Copy link
Member

@janpio janpio commented Sep 5, 2018

@knight9999 Could you please summarize your changes in the PR title? Thanks.

@janpio janpio moved this from 🐣 New PR / Untriaged to 👷 Blocked: Work in Progress in Apache Cordova: Tooling Pull Requests Sep 5, 2018
@knight9999 knight9999 changed the title CB-14108: CB-14108: fix incorrect count in config_munge in ios.json and android.json Sep 6, 2018
@knight9999
Copy link
Contributor Author

@knight9999 knight9999 commented Sep 6, 2018

@janpio I see. I have updated the title.

Apache Cordova: Tooling Pull Requests automation moved this from 👷 Blocked: Work in Progress to ✅ Approved, waiting for Merge Sep 6, 2018
@erisu
erisu approved these changes Sep 6, 2018
Copy link
Contributor

@raphinesse raphinesse left a comment

I don't like adding package-lock.json as a by product. Or has this actually something to do with this PR?

Copy link
Contributor

@raphinesse raphinesse left a comment

Looks like there are some linting errors. Sorry for spotting them just now.

Apache Cordova: Tooling Pull Requests automation moved this from ✅ Approved, waiting for Merge to 🙅 Pending Approval Sep 6, 2018
@knight9999 knight9999 force-pushed the cordova-develop:CB-14108 branch from da206a9 to ef36092 Sep 6, 2018
@raphinesse raphinesse dismissed their stale review Sep 9, 2018

Linting errors are gone

Apache Cordova: Tooling Pull Requests automation moved this from 🙅 Pending Approval to ✅ Approved, waiting for Merge Sep 9, 2018
@dpogue
dpogue approved these changes Sep 11, 2018
@dpogue dpogue merged commit ce3801a into apache:master Sep 11, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Apache Cordova: Tooling Pull Requests automation moved this from ✅ Approved, waiting for Merge to 🏆 Merged, waiting for Release Sep 11, 2018
@dpogue
Copy link
Member

@dpogue dpogue commented Apr 17, 2019

@knight9999 The changes here (specifically around unusedConfigMunge ) seem to be causing config-file and edit-config directives in the project config.xml to be skipped sometimes: apache/cordova-android#704

@ekrapfl
Copy link

@ekrapfl ekrapfl commented Jun 11, 2019

@knight9999 and @dpogue
Is anyone looking into this? It is a pretty major issue that is preventing us from using Cordova 9, which means we are stuck using the horribly slow prepare in Cordova 8. This appears to be the only thing preventing us from cutting 10+ minutes off of our build times. Any assistance would be appreciated. I would love to help, but I don't have the slightest clue what is going on in this commit, so I don't think I am qualified :)

@dpogue
Copy link
Member

@dpogue dpogue commented Jun 11, 2019

No update from me, I have absolutely no time to work on Cordova stuff these days.

erisu added a commit that referenced this pull request Aug 21, 2019
* Revert "CB-14108: fix incorrect count in config_munge in ios.json and android.json"

This reverts commit ce3801a.

* chore: eslint object-curly-spacing fix
* add CordovaError spec
* chore: fix eslint warnings for CordovaError spec
* fix CordovaError spec test
* chore: added additional AS project path check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Apache Cordova: Tooling Pull Requests
🏆 Merged, waiting for Release
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants