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-14099 osx: Fixed Resolve Config Path for OSX #32

Closed
wants to merge 1 commit into from

Conversation

erisu
Copy link
Member

@erisu erisu commented Jun 26, 2018

Platforms affected

OSX

What does this PR do?

Updates the Resolve Config Path for OSX.

I have identified that the initial build was successful but the consecutive builds fail because it updated the incorrect config.xml file.

I was able to narrow down this issue coming from the resolveConfigFilePath method. This method currently handles special cases for Android, Ubuntu, and iOS. For all other platforms, it would fall back to glob for the config.xml. This is what was happens for OSX.

When building for the first time, you have installed a fresh copy of the OSX platform.
At this time, when globbing, it will find the correct config.xml file located in /.tests/<APP_NAME>/platforms/osx/<APP_NAME>/config.xml.

In the consecutive build, when globbing, it finds and updates the incorrect config.xml file. The file it finds is: /.tests/osx/<APP_NAME>/platforms/osx/build/<APP_NAME>.app/Contents/Resources/config.xml
This is because of the folder/file order structure and globbing nature. Since this file was created after the first build (build history), it happens to find this file first.

What testing has been done on this change?

I also followed the reported issue steps:

  • $ cordova create myapp org.apache.myapp myapp
  • $ cordova platform add osx
    • I used a cloned version of cordova-osx@master with modified package.json to use cordova-common#CB-14099 with my fix.
  • $ cordova plugin add cordova-plugin-file
  • $ cordova run osx
    • I ran this step 3x times.

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 commented Jun 26, 2018

Codecov Report

Merging #32 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
- Coverage   85.63%   85.62%   -0.01%     
==========================================
  Files          19       19              
  Lines        1747     1746       -1     
  Branches      367      367              
==========================================
- Hits         1496     1495       -1     
  Misses        251      251
Impacted Files Coverage Δ
src/ConfigChanges/ConfigFile.js 89.78% <100%> (-0.08%) ⬇️

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 c104401...6c0ca12. Read the comment docs.

Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

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

👍

I wonder if we should try to do anything (far outside the scope of this PR) about the fact that "OS X" has been rebranded as "macOS"

@tripodsan
Copy link

+1

@erisu
Copy link
Member Author

erisu commented Jun 27, 2018

@dpogue I was also thinking about that. Even when I saw the if condition for Ubuntu, I wanted to delete it because it had been deprecated, but also had to leave it because it was out-of-scope.

If its a good idea, we could scope out some changes and code cleanup. I wouldn't mind working on it.

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.

None yet

4 participants