Fix issues with pbxFile extension and addTargetDependency sections #12

kspearrin · 2018-09-05T17:10:47Z

This PR fixes a few issues I discovered while using this library to add an app extension to a pbxproject.

new pbxFile('AppExtension', { explicitFileType: '"wrapper.app-extension"'}) gets indirectly called when using proj.addTarget('AppExtension', 'app_extension'). This results in an incorrect extension being added to the basename when trying to determine the extension from defaultExtension (detectType sets it to the literal string "unknown" which fails falsey checks). The result would end up being AppExtension.undefined instead of the correct AppExtension.appex. Also corrected a missing unquoted() call in the defaultExtension() function which also led to the same failure.
addTargetDependency() is indirectly called as well from proj.addTarget('AppExtension', 'app_extension'). In my project, PBXTargetDependency and PBXContainerItemProxy sections did not yet exist which results in the necessary dependencies not being set up properly for building the extension with the main app. Create these sections if they do not exist.
In the various functions that build out buildSettings, it checks for the PRODUCT_NAME to be the same as the project's product name. This never matches if you are using these functions on the app extension, since it has a different PRODUCT_NAME. Allow a product name to be passed as an option to these functions.
Added showEnvVarsInLog option to pbxShellScriptBuildPhaseObj. Passing this a false will set showEnvVarsInLog = 0.

All tests (and the newly added) pass.

janpio · 2018-09-05T17:13:53Z

Offtopic:

This PR fixes two issues I discovered while using this library to add an app extension to a pbxproject

Public repository somewhere? I remember some people asking about something like that...

kspearrin · 2018-09-05T17:16:39Z

@janpio We are looking into moving to NativeScript (from Xamarin) with our app here: https://github.com/bitwarden/mobile/tree/ns2 . Though most of what is going on right now is just discovery so not all work is committed.

janpio · 2018-09-05T17:24:18Z

Ah ok, let me know if this works out with cordova-node-xcode and that code is online.

brody4hire · 2018-12-09T17:27:02Z

What is the status of this proposal?

n1ru4l

I added some comments, the overall functionality seems fine, but please us strict equal checks instead of the abstract equal checks

n1ru4l · 2018-12-09T20:38:11Z

test/pbxFile.js

@@ -278,5 +278,13 @@ exports['settings'] = {

        test.deepEqual({COMPILER_FLAGS:'"-std=c++11 -fno-objc-arc"'}, sourceFile.settings);
        test.done();
+    },
+
+    'should have extension if {explicitFileType:"blah"} specified': function (test) {


the test name here does not match the actual content of the test

lib/pbxProject.js

test/pbxFile.js

brody4hire · 2018-12-09T23:07:49Z

Thanks @n1ru4l for reviewing. It looks like we do want at least some of the proposed changes but I still don't really understand everything that is changed here.

Co-Authored-By: kspearrin <[email protected]>

kspearrin · 2018-12-10T15:10:23Z

@n1ru4l Suggested changes applied.

brody4hire

Several changes in lib/pbxFile.js are not covered by testing. Reverting these proposed changes would not cause any test failures
None of the changes in lib/pbxProject.js are covered by testing. Reverting all of the proposed changes in this file would not cause any test failures.

I would favor merging the first few changes in lib/pbxFile.js, which seem to be properly tested, and dealing with the other changes in one or more separate PRs.

Also +1 for changing != to !== but I would favor making this kind of fix, throughout this project, in its own PR.

brody4hire · 2018-12-10T16:06:27Z

lib/pbxFile.js

                 return extension;
        }
    }
 }

 function defaultEncoding(fileRef) {
-    var filetype = fileRef.lastKnownFileType || fileRef.explicitFileType,
+    var filetype = fileRef.lastKnownFileType && fileRef.lastKnownFileType != DEFAULT_FILETYPE ?
+            fileRef.lastKnownFileType : fileRef.explicitFileType,


This change is not covered by testing.

brody4hire · 2018-12-10T16:14:06Z

lib/pbxFile.js

@@ -119,7 +121,8 @@ function defaultEncoding(fileRef) {

 function detectGroup(fileRef, opt) {
    var extension = path.extname(fileRef.basename).substring(1),
-        filetype = fileRef.lastKnownFileType || fileRef.explicitFileType,
+        filetype = fileRef.lastKnownFileType && fileRef.lastKnownFileType != DEFAULT_FILETYPE ?
+            fileRef.lastKnownFileType : fileRef.explicitFileType,


This change is not covered by testing.

brody4hire · 2018-12-10T16:14:18Z

lib/pbxFile.js

@@ -139,7 +142,8 @@ function detectGroup(fileRef, opt) {

 function detectSourcetree(fileRef) {

-    var filetype = fileRef.lastKnownFileType || fileRef.explicitFileType,
+    var filetype = fileRef.lastKnownFileType && fileRef.lastKnownFileType != DEFAULT_FILETYPE ?
+            fileRef.lastKnownFileType : fileRef.explicitFileType,


This change is not covered by testing.

brody4hire · 2018-12-10T16:14:23Z

lib/pbxFile.js

@@ -158,7 +162,8 @@ function detectSourcetree(fileRef) {
 }

 function defaultPath(fileRef, filePath) {
-    var filetype = fileRef.lastKnownFileType || fileRef.explicitFileType,
+    var filetype = fileRef.lastKnownFileType && fileRef.lastKnownFileType != DEFAULT_FILETYPE ?
+            fileRef.lastKnownFileType : fileRef.explicitFileType,


This change is not covered by testing.

brody4hire · 2018-12-10T16:24:42Z

I would like to add one more tip that you should always make a new branch when proposing changes to keep your master branch clean and give us a more sensical commit message in case of a merge commit.

Thanks @kspearrin for your efforts.

where pbxFile extension was being set to 'undefined' and add new test case to verify the bug fix NOTE: These changes were originally part of apache#12 (cordova-node-xcode PR apache#12), extracted here by @brodybits (Christopher J. Brody). Co-authored-by: Kyle Spearrin <[email protected]> Co-authored-by: Christopher J. Brody <[email protected]>

where pbxFile extension was being set to 'undefined' and add new test case to verify the bug fix with a quick test fix by @brodybits (Christopher J. Brody) for the sake of consistency with the other test cases NOTE: These changes were originally part of #12 (cordova-node-xcode PR #12), extracted here by @brodybits. Co-authored-by: Kyle Spearrin <[email protected]> Co-authored-by: Christopher J. Brody <[email protected]>

brody4hire · 2018-12-12T04:04:01Z

The changes to fix the pbxFiile extension issue were merged in PR #37, with due credit given to @kspearrin. I am closing this one, raised #37 to track the other issues that should be fixed. We really cannot make the other proposed fixes without test coverage, otherwise we risk things breaking at some random time in the future.

The strict equality checks will be done when we introduce the eslint updates (#36).

kspearrin added 2 commits September 5, 2018 12:50

fix issue where pbxFile extension was being set to 'undefined'

e82e2ab

create TargetDependency and ContainerItemProxy sections if not defined

2e09d78

kspearrin added 4 commits September 6, 2018 17:24

showEnvVarsInLog and productName support

24fb9dd

restore back DEFAULT_FILETYPE and then check its value

f62d7be

restore back unknown test

0847d0e

arg to filter by product_name when adding/removing build prop

83f7335

brody4hire mentioned this pull request Dec 9, 2018

Major update proposal #20

Open

11 tasks

n1ru4l suggested changes Dec 9, 2018

View reviewed changes

brody4hire reviewed Dec 9, 2018

View reviewed changes

test/pbxFile.js Outdated Show resolved Hide resolved

n1ru4l and others added 2 commits December 10, 2018 10:03

Apply suggestions from code review

206b864

Co-Authored-By: kspearrin <[email protected]>

update test name for appex

baf9a4f

brody4hire suggested changes Dec 10, 2018

View reviewed changes

brody4hire mentioned this pull request Dec 11, 2018

Resolve issue with pbxFile extension #31

Merged

brody4hire mentioned this pull request Dec 12, 2018

Fix issues related to targetDependency, default path, etc. as proposed in PR #12 #37

Open

brody4hire closed this Dec 12, 2018

rgomezp mentioned this pull request Dec 4, 2021

Beta: iOS not yet fully functional OneSignal/onesignal-expo-plugin#21

Closed

jkasten2 mentioned this pull request Dec 4, 2021

BUG - addTargetDependency - If project doesn't contain PBXTargetDependency or PBXContainerItemProxy #121

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues with pbxFile extension and addTargetDependency sections #12

Fix issues with pbxFile extension and addTargetDependency sections #12

kspearrin commented Sep 5, 2018 •

edited

Loading

janpio commented Sep 5, 2018

kspearrin commented Sep 5, 2018

janpio commented Sep 5, 2018

brody4hire commented Dec 9, 2018

n1ru4l left a comment

n1ru4l Dec 9, 2018

brody4hire commented Dec 9, 2018

kspearrin commented Dec 10, 2018 •

edited

Loading

brody4hire left a comment

brody4hire Dec 10, 2018

brody4hire Dec 10, 2018

brody4hire Dec 10, 2018

brody4hire Dec 10, 2018

brody4hire commented Dec 10, 2018

brody4hire commented Dec 12, 2018

Fix issues with pbxFile extension and addTargetDependency sections #12

Fix issues with pbxFile extension and addTargetDependency sections #12

Conversation

kspearrin commented Sep 5, 2018 • edited Loading

janpio commented Sep 5, 2018

kspearrin commented Sep 5, 2018

janpio commented Sep 5, 2018

brody4hire commented Dec 9, 2018

n1ru4l left a comment

Choose a reason for hiding this comment

n1ru4l Dec 9, 2018

Choose a reason for hiding this comment

brody4hire commented Dec 9, 2018

kspearrin commented Dec 10, 2018 • edited Loading

brody4hire left a comment

Choose a reason for hiding this comment

brody4hire Dec 10, 2018

Choose a reason for hiding this comment

brody4hire Dec 10, 2018

Choose a reason for hiding this comment

brody4hire Dec 10, 2018

Choose a reason for hiding this comment

brody4hire Dec 10, 2018

Choose a reason for hiding this comment

brody4hire commented Dec 10, 2018

brody4hire commented Dec 12, 2018

kspearrin commented Sep 5, 2018 •

edited

Loading

kspearrin commented Dec 10, 2018 •

edited

Loading