-
Notifications
You must be signed in to change notification settings - Fork 990
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
[minor] fix *-Info.plist & config.xml functionality #795
base: master
Are you sure you want to change the base?
[minor] fix *-Info.plist & config.xml functionality #795
Conversation
…sger/cordova-ios into plist-config-fixes-and-improvements
Co-authored-by: Leo Qiu <[email protected]> Co-authored-by: Christopher J. Brody <[email protected]> Co-authored-by: エリス <[email protected]>
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@brodybits Is this ticket fixing a bug and also considered to be breaking because you are searching for What if you change the 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 ( You could still create a second breaking PR after this is merged into master to change only the |
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. |
I now wonder if <apache/cordova-common/pull/148> may be related |
@brodybits I can confirm that the PR you mentioned does not fix this issue, as I am still seeing this in |
const workspaceName = | ||
fs.readdirSync(project_dir).find(d => d.includes('.xcworkspace')) || ''; | ||
|
||
const projectName = workspaceName.replace('.xcworkspace', ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noticed we have a getIOSProjectname
function in cordova-common
, maybe you could use it here?
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.
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 8dfeedc, 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