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

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

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

brodybits
Copy link
Contributor

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

@codecov-io
Copy link

codecov-io commented Feb 19, 2020

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%) ⬆️

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.

@brodybits brodybits added the bug label Feb 20, 2020
@brodybits brodybits changed the title breaking: *-Info.plist config fixes and improvements breaking: *-Info.plist config bug fixes & improvements Feb 20, 2020
@brodybits brodybits changed the title breaking: *-Info.plist config bug fixes & improvements breaking: fix *-Info.plist & config.xml functionality Feb 20, 2020
@brodybits brodybits added this to the 6.0.0 milestone Feb 20, 2020
@brodybits brodybits modified the milestones: 6.0.0, 7.0.0 Jun 5, 2020
@erisu
Copy link
Member

erisu commented Jun 12, 2020

@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...

@brodybits brodybits marked this pull request as draft June 12, 2020 16:38
@brodybits brodybits changed the title breaking: fix *-Info.plist & config.xml functionality fix *-Info.plist & config.xml functionality Jun 12, 2020
@brodybits brodybits changed the title fix *-Info.plist & config.xml functionality [minor] fix *-Info.plist & config.xml functionality Jun 12, 2020
@brodybits
Copy link
Contributor Author

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.

@brodybits
Copy link
Contributor Author

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

@Menardi
Copy link

Menardi commented Oct 10, 2020

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

const workspaceName =
fs.readdirSync(project_dir).find(d => d.includes('.xcworkspace')) || '';

const projectName = workspaceName.replace('.xcworkspace', '');
Copy link

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?

@dpogue dpogue removed this from the 7.0.0 milestone Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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