Fix error parsing plist_file_entry when adding extension in xcode #765

leogoesger · 2020-01-14T00:50:12Z

Closes #764

NiklasMerz · 2020-01-14T09:47:15Z

Thanks for submitting the PR. Now we just need to get working tests for this change :-)

@leogoesger Please check the failing tests.

leogoesger · 2020-01-16T19:59:23Z

@nikhilkh I corrected the linter error and unit test. Thanks

codecov-io · 2020-01-16T20:15:51Z

Codecov Report

Merging #765 into master will increase coverage by <.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #765      +/-   ##
==========================================
+ Coverage   74.42%   74.42%   +<.01%     
==========================================
  Files          11       11              
  Lines        1830     1834       +4     
==========================================
+ Hits         1362     1365       +3     
- Misses        468      469       +1

Impacted Files	Coverage Δ
bin/templates/scripts/cordova/lib/projectFile.js	`93.33% <83.33%> (-1.31%)`	⬇️

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 266d339...dd4a72e. Read the comment docs.

NiklasMerz

Great! CI looks better now.

Looks like you accidentally added package-lock.json in your last commit.

erisu

I left some feedback.

bin/templates/scripts/cordova/lib/projectFile.js

erisu · 2020-01-17T00:09:19Z

bin/templates/scripts/cordova/lib/projectFile.js

-    const xcBuildConfiguration = xcodeproj.pbxXCBuildConfigurationSection();
-    const plist_file_entry = _.find(xcBuildConfiguration, entry => entry.buildSettings && entry.buildSettings.INFOPLIST_FILE);
+    var projectName = '';
+    if (fs.readdirSync(project_dir).find(d => d.includes('.xcworkspace'))) {


I would recommend pulling this out into its own variable. No need to call this twice.. It would lower the number of file disk reads (readdir) and loops.

E.g.

const workspaceCollection = fs.readdirSync(project_dir).find(d => d.includes('.xcworkspace')); if (workspaceCollection) { projectName = workspaceCollection.replace('.xcworkspace', ''); }

bin/templates/scripts/cordova/lib/projectFile.js

erisu · 2020-01-17T01:07:30Z

bin/templates/scripts/cordova/lib/projectFile.js

+    var plist_file_entry = _.find(xcBuildConfiguration, function (entry) {
+        return (
+            entry.buildSettings &&
+            entry.buildSettings.INFOPLIST_FILE &&
+            projectName ? entry.buildSettings.INFOPLIST_FILE.includes(projectName) : true
+        );
+    });
    const plist_file = path.join(project_dir, plist_file_entry.buildSettings.INFOPLIST_FILE.replace(/^"(.*)"$/g, '$1').replace(/\\&/g, '&'));


I am not sure if this produces correct results always.

What happens if there is two -Info.plist that has very close identical names.

For example lets say our projectName is MyApp. The expected plist would be MyApp-Info.plist. Now what if there was another plist named MyAppExtension-Info.plist. This would also match from what is described above. It seems we are assuming that the order of the xcBuildConfiguration always returns our project first before other.

Additionally, we are trying to remove the need of using third-party libraries such as lodash. I would recommend rewriting this area are such.

const { buildSettings: { INFOPLIST_FILE : infoPlistFile } } = Object.values(xcBuildConfiguration) .filter(({ buildSettings }) => ( buildSettings && buildSettings.INFOPLIST_FILE && projectName ? buildSettings.INFOPLIST_FILE.match(new RegExp(`/${projectName}-Info.plist"$`)) : false ))[0]; const plist_file = path.join(project_dir, infoPlistFile.replace(/^"(.*)"$/g, '$1').replace(/\\&/g, '&'));

If you use the above code, would recommend confirming that it continues to produce the same results as you expect.

The code above will always expect to find at least one and in my case I always expect actually two results (Debug and Release). Since the InfoPlist path will be identical, I only take the first one [0].

Thanks, I have never worked in xcode, nor in Cordova before. As far as JS goes,

MyAppExtension-Info.plist".includes("MyApp-Info.plist") is false. The only way to return true would be ExtensionMyApp-Info.plist".includes("MyApp-Info.plist"). Instead of doing the check, it might be easier to just use

entry.buildSettings.INFOPLIST_FILE === `${projectName}-Info.plist`

Here is an actual INFOPLIST_FILE value that I grabbed from a new empty project.

"MyApp/MyApp-Info.plist"

Please note that the surrounding double quotes are also in string.

Update: Here are some more sample INFOPLIST_FILE values. On the blank project I added some random watch extensions to create more data.

"MyAppWatchExtension Extension/Info.plist"

"SomeOtherWatchExtension WatchKit Extension/Info.plist"

"SomeOtherWatchExtension WatchKit App/Info.plist"

I am not sure why this one didn't have the double quotation wrapper like the other examples, but it seems there can be cases where its not wrapped.

MyAppWatchExtension/Info.plist

Also, just wanted to point out that you wrote above:

The code you showed in comment is true,
MyAppExtension-Info.plist".includes("MyApp-Info.plist")

But what you wrote in code didn't reflect that previously.

entry.buildSettings.INFOPLIST_FILE.includes(projectName)

for example it would have been like these which would have matched in all cases.

"MyApp/MyApp-Info.plist"".includes("MyApp")

MyAppWatchExtension Extension/Info.plist

But I do see it is updated to ${projectName}/${projectName}-Info.plist now.

sou2 · 2020-02-14T09:55:02Z

bin/templates/scripts/cordova/lib/projectFile.js

+        return (
+            entry.buildSettings &&
+            entry.buildSettings.INFOPLIST_FILE &&
+            projectName ? entry.buildSettings.INFOPLIST_FILE.includes(`${projectName}/${projectName}-Info.plist`) : true


The ternary operator (? :) needs to be enclosed in parenthesis. It has lower priority than the && operator. Else, it crashes on my app because it returns true for an entry that has no buildSettings.

brody4hire · 2020-02-17T15:07:43Z

bin/templates/scripts/cordova/lib/projectFile.js

+        return (
+            entry.buildSettings &&
+            entry.buildSettings.INFOPLIST_FILE &&
+            projectName ? entry.buildSettings.INFOPLIST_FILE.includes(`${projectName}/${projectName}-Info.plist`) : true


How about this idea:

Suggested change

projectName ? entry.buildSettings.INFOPLIST_FILE.includes(`${projectName}/${projectName}-Info.plist`) : true

!(projectName && entry.buildSettings.INFOPLIST_FILE.includes(`${projectName}/${projectName}-Info.plist`))

brody4hire · 2020-02-17T15:08:19Z

What is the status of this proposal?

leogoesger · 2020-02-18T16:42:13Z

I included the suggested edits

brody4hire · 2020-02-18T17:23:54Z

Thanks. I restarted a Travis CI build job due to a test timeout, it seems to be green now. I have some other remarks coming up.

brody4hire

I am very sorry to say that there are some things that need to be fixed before we can consider merging this proposal:

1: As I said in PR #792, I discovered that we need to add a test case for some existing code. Someone needs to add a test case that will fail if the internal filter on *-Info.plist is broken for any reason.

2: Extra package-lock.json file is included in this PR. Please remove it.

3: The changes proposed are not properly covered in the test suite. We need a test case that can reproduce the issue described in #764 and ensure that the issue does not reappear in the future.

Another thing is that when I tried removing a few lines from the proposed change, npm test did not fail in my work area:

diff --git a/bin/templates/scripts/cordova/lib/projectFile.js b/bin/templates/scripts/cordova/lib/projectFile.js
index 84200517..862addd1 100644
--- a/bin/templates/scripts/cordova/lib/projectFile.js
+++ b/bin/templates/scripts/cordova/lib/projectFile.js
@@ -42,9 +42,6 @@ function parseProjectFile (locations) {
 
     let projectName = '';
     const workspaceCollection = fs.readdirSync(project_dir).find(d => d.includes('.xcworkspace'));
-    if (workspaceCollection) {
-        projectName = fs.readdirSync(project_dir).find(d => d.includes('.xcworkspace')).replace('.xcworkspace', '');
-    }
     const xcBuildConfiguration = xcodeproj.pbxXCBuildConfigurationSection();
     const plist_file_entry = _.find(xcBuildConfiguration, function (entry) {
         return (

brody4hire · 2020-02-19T03:19:56Z

bin/templates/scripts/cordova/lib/projectFile.js

+        return (
+            entry.buildSettings &&
+            entry.buildSettings.INFOPLIST_FILE &&
+            !(projectName && entry.buildSettings.INFOPLIST_FILE.includes(`${projectName}/${projectName}-Info.plist`))


Needs to be corrected (I got it wrong in my suggestion):

Suggested change

!(projectName && entry.buildSettings.INFOPLIST_FILE.includes(`${projectName}/${projectName}-Info.plist`))

!(projectName && !entry.buildSettings.INFOPLIST_FILE.includes(`${projectName}/${projectName}-Info.plist`))

or better yet:

Suggested change

!(projectName && entry.buildSettings.INFOPLIST_FILE.includes(`${projectName}/${projectName}-Info.plist`))

(!projectName ||

entry.buildSettings.INFOPLIST_FILE.includes(`${projectName}/${projectName}-Info.plist`))

brody4hire

I would like to recommend that we close this PR in favor of PR #795, which includes these updates, added test cases, and some more rework. I would like to thank @leogoesger for your efforts in reporting #764 and contributing this proposed solution. @leogoesger will definitely get co-author credit once we merge PR #795.

leogoesger · 2020-02-20T15:22:10Z

@brodybits Thanks, and no problem :)

See apache#765 (comment) Co-authored-by: エリス <[email protected]>

plist_file_entry paring error

3252351

fix unit test and linter

dd4a72e

leogoesger mentioned this pull request Jan 16, 2020

could not find -Info.plist file, or config.xml file after adding extension, cordova build, with FIX #764

Closed

3 tasks

NiklasMerz requested changes Jan 16, 2020

View reviewed changes

erisu requested changes Jan 17, 2020

View reviewed changes

fix const, let and project name

b281150

sou2 suggested changes Feb 14, 2020

View reviewed changes

brody4hire reviewed Feb 17, 2020

View reviewed changes

remove the use of ternary operator

30cdacc

brody4hire mentioned this pull request Feb 18, 2020

[BREAKING TEST] try removing internal -Info.plist filter - DO NOT MERGE #792

Closed

brody4hire suggested changes Feb 18, 2020

View reviewed changes

brody4hire reviewed Feb 19, 2020

View reviewed changes

brody4hire mentioned this pull request Feb 19, 2020

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

Closed

brody4hire suggested changes Feb 19, 2020

View reviewed changes

leogoesger closed this Feb 20, 2020

NiklasMerz added a commit to GEDYSIntraWare/cordova-ios that referenced this pull request Feb 24, 2020

Alternative projectname replace

f191116

See apache#765 (comment) Co-authored-by: エリス <[email protected]>

NiklasMerz added a commit to GEDYSIntraWare/cordova-ios that referenced this pull request Feb 24, 2020

Alternative projectname replace

19cf34c

See apache#765 (comment) Co-authored-by: エリス <[email protected]>

brody4hire mentioned this pull request Apr 16, 2020

internal *-Info.plist filter behavior not properly tested #826

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error parsing plist_file_entry when adding extension in xcode #765

Fix error parsing plist_file_entry when adding extension in xcode #765

leogoesger commented Jan 14, 2020 •

edited by NiklasMerz

Loading

NiklasMerz commented Jan 14, 2020 •

edited

Loading

leogoesger commented Jan 16, 2020

codecov-io commented Jan 16, 2020 •

edited

Loading

NiklasMerz left a comment

erisu left a comment

erisu Jan 17, 2020

erisu Jan 17, 2020

leogoesger Jan 17, 2020 •

edited

Loading

erisu Jan 17, 2020

erisu Jan 17, 2020 •

edited

Loading

erisu Jan 17, 2020 •

edited

Loading

leogoesger Feb 18, 2020

sou2 Feb 14, 2020

brody4hire Feb 17, 2020

brody4hire commented Feb 17, 2020

leogoesger commented Feb 18, 2020

brody4hire commented Feb 18, 2020

brody4hire left a comment

brody4hire Feb 19, 2020

brody4hire left a comment

leogoesger commented Feb 20, 2020

	projectName ? entry.buildSettings.INFOPLIST_FILE.includes(`${projectName}/${projectName}-Info.plist`) : true
	!(projectName && entry.buildSettings.INFOPLIST_FILE.includes(`${projectName}/${projectName}-Info.plist`))

	!(projectName && entry.buildSettings.INFOPLIST_FILE.includes(`${projectName}/${projectName}-Info.plist`))
	!(projectName && !entry.buildSettings.INFOPLIST_FILE.includes(`${projectName}/${projectName}-Info.plist`))

	!(projectName && entry.buildSettings.INFOPLIST_FILE.includes(`${projectName}/${projectName}-Info.plist`))
	(!projectName \|\|
	entry.buildSettings.INFOPLIST_FILE.includes(`${projectName}/${projectName}-Info.plist`))

Fix error parsing plist_file_entry when adding extension in xcode #765

Fix error parsing plist_file_entry when adding extension in xcode #765

Conversation

leogoesger commented Jan 14, 2020 • edited by NiklasMerz Loading

NiklasMerz commented Jan 14, 2020 • edited Loading

leogoesger commented Jan 16, 2020

codecov-io commented Jan 16, 2020 • edited Loading

Codecov Report

NiklasMerz left a comment

Choose a reason for hiding this comment

erisu left a comment

Choose a reason for hiding this comment

erisu Jan 17, 2020

Choose a reason for hiding this comment

erisu Jan 17, 2020

Choose a reason for hiding this comment

leogoesger Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

erisu Jan 17, 2020

Choose a reason for hiding this comment

erisu Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

erisu Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

leogoesger Feb 18, 2020

Choose a reason for hiding this comment

sou2 Feb 14, 2020

Choose a reason for hiding this comment

brody4hire Feb 17, 2020

Choose a reason for hiding this comment

brody4hire commented Feb 17, 2020

leogoesger commented Feb 18, 2020

brody4hire commented Feb 18, 2020

brody4hire left a comment

Choose a reason for hiding this comment

brody4hire Feb 19, 2020

Choose a reason for hiding this comment

brody4hire left a comment

Choose a reason for hiding this comment

leogoesger commented Feb 20, 2020

leogoesger commented Jan 14, 2020 •

edited by NiklasMerz

Loading

NiklasMerz commented Jan 14, 2020 •

edited

Loading

codecov-io commented Jan 16, 2020 •

edited

Loading

leogoesger Jan 17, 2020 •

edited

Loading

erisu Jan 17, 2020 •

edited

Loading

erisu Jan 17, 2020 •

edited

Loading