cordova-ios icon indicating copy to clipboard operation
cordova-ios copied to clipboard

[minor] fix *-Info.plist & config.xml functionality

Open brody4hire opened this issue 5 years ago • 5 comments

This is proposed to include and supersede PR #765, as a solution for #764, with some additional fixes and test updates that I think should be included at the same time.

  • updates and improvements to unit testing of projectFile parse method, now with xcworkspace included in the "ios-config-xml" fixture
  • include all changes from PR #765 so far, with possible solution to issue #764
  • some followup fixes to PR #765
  • add test cases for the code updated in PR #765
  • some more general improvements

This proposal should be considered breaking since it now depends on existence of xcworkspace with the correct name. This should have been present in generated platforms/ios project for years.

I have discussed some other related issues with plists in #793, not sure whether or not this proposal would resolve any such related issues.

I would like to have multiple reviews before getting this proposal merged. I think it should be good to merge this with a squash merge, with all co-authored-by credits that I gave in 8dfeedc8f754f7638b96b6677a0f34a4d246c8c5, maybe with a rebase to cleanup some of the items that will go into the commit message.

closes #765 (PR #765) resolves #764

/cc @leogoesger

brody4hire avatar Feb 19 '20 09:02 brody4hire

Codecov Report

Merging #795 into master will increase coverage by 0.11%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #795      +/-   ##
==========================================
+ Coverage   74.36%   74.47%   +0.11%     
==========================================
  Files          13       13              
  Lines        1845     1853       +8     
==========================================
+ Hits         1372     1380       +8     
  Misses        473      473
Impacted Files Coverage Δ
bin/templates/scripts/cordova/lib/projectFile.js 95.31% <100%> (+0.66%) :arrow_up:

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 4ac1552...b2496e3. Read the comment docs.

codecov-io avatar Feb 19 '20 10:02 codecov-io

@brodybits Is this ticket fixing a bug and also considered to be breaking because you are searching for xcworkspace?

What if you change the xcworkspace search to xcodeproj?

Would this still fix the issue and change the PR's type from breaking to patch or minor?

Of corse the conflict will also need to be corrected.

There is a patch release (6.0.1) being prepared. If this PR is resolving a bug and can be altered to be a patch instead of breaking, you might be able to get this PR in.

You could still create a second breaking PR after this is merged into master to change only the xcodeproj to xcworkspace...

erisu avatar Jun 12 '20 05:06 erisu

The good news is that I would no longer consider this to be a breaking change. If I would remove the xcworkspace file and then build from the command line, it results in an error from xcodebuild. (I was able to remove the xcworkspace file and then build & run from Xcode, which I think is a side point.)

I do think this is too much change for a patch, though, should probably target a minor release.

I did try merging. The changes did not seem to work after merging, and some of the tests need rework due to removing shelljs. I think we should really rebase and rework. I will try to fit this in as soon as I can.

brody4hire avatar Jun 12 '20 17:06 brody4hire

I now wonder if <apache/cordova-common/pull/148> may be related

brody4hire avatar Jun 16 '20 15:06 brody4hire

@brodybits I can confirm that the PR you mentioned does not fix this issue, as I am still seeing this in [email protected]. Though the cause of the issues is the same (having multiple Info.plist) files, their fixes are separate.

Menardi avatar Oct 10 '20 16:10 Menardi