CB-12397 fix .gitignore for plugins & platforms (cordova-create part)

[brodybits]

Platforms affected

All

What does this PR do?

Workaround for Apache Cordova CB-12397: include .gitignore from template_src subdirectory of cordova-app-hello-world, needed due to the npm .gitignore/.npmignore behavior discussed in:

• Rename .gitignore to .npmignore in package if no .npmignore found npm/npm#1862
• NPM Publish Doesn't Include .gitignore npm/npm#3763
• npm pack/publish option to not rename or keep a copy of .gitignore files npm/npm#7252

NOTE: Another part of the fix for Apache Cordova CB-12397 is in https://github.com/brodybits/cordova-app-hello-world/tree/cb-12397, to be raised after this PR.

Additional commits:

• cleanup: remove trailing whitespace
• general updates to test suite

What testing has been done on this change?

Unit test

Test suite updated to verify that .gitignore is included in the generated app when the standard cordova-app-hello-world template is used.

Cordova CLI test

• Using cordova-cli test version in https://github.com/brodybits/cordova-cli/tree/cb-12397-devtest1 which uses local test installation of cordova-lib from https://github.com/brodybits/cordova-lib/tree/cb-12397-devtest1 due to limitations discussed in Allow subdirectories within git repos in npm install  npm/npm#2974.
• cordova-lib in https://github.com/brodybits/cordova-lib/tree/cb-12397-devtest1 references cordova-create in https://github.com/brodybits/cordova-create/tree/cb-12397-devtest1 which includes this fix.
• cordova-create in https://github.com/brodybits/cordova-create/tree/cb-12397-devtest1 references cordova-app-hello-world in https://github.com/brodybits/cordova-app-hello-world/tree/cb-12397-devtest1 which includes the cordova-app-hello-world fix in https://github.com/brodybits/cordova-app-hello-world/tree/cb-12397.

Testing with results and other output on Windows:

C:\Users\Chris\Documents\chris-work\cb-12397-dev
λ npm install -g https://github.com/brodybits/cordova-cli#cb-12397-devtest1
npm WARN deprecated node-uuid@1.4.8: Use uuid module instead
C:\Users\Chris\nvs\node\7.10.0\x64\cordova -> C:\Users\Chris\nvs\node\7.10.0\x64\node_modules\cordova\bin\cordova
C:\Users\Chris\nvs\node\7.10.0\x64
`-- cordova@7.0.2-cb-12397-devtest1  (git+https://github.com/brodybits/cordova-cli.git#9f4648d5565b098434187aa3da251beca8d68dae)
[...]
C:\Users\Chris\Documents\chris-work\cb-12397-dev
λ cordova --version
7.0.2-cb-12397-devtest1

C:\Users\Chris\Documents\chris-work\cb-12397-dev
λ cordova create cb-12397-testapp1
Creating a new cordova project.

C:\Users\Chris\Documents\chris-work\cb-12397-dev
λ ls -a !$
History expansion: ls -a cb-12397-testapp1
./  ../  .gitignore  .npmignore  config.xml  hooks/  package.json  platforms/  plugins/  res/  www/

C:\Users\Chris\Documents\chris-work\cb-12397-dev
λ cat cb-12397-testapp1\.gitignore
# macOS
.DS_Store

# Generated by Cordova
plugins
platforms

C:\Users\Chris\Documents\chris-work\cb-12397-dev
λ

.gitignore is now present in the generated app, with plugins and platforms as needed

Checklist

• Reported an issue in the JIRA database
• Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
• Added automated test coverage as appropriate for this change.

[brodybits]

I just commented out some extra tests to get these changes to pass, now looking forward to review and feedback.

[brodybits]

I would still like to see this change integrated, new JIRA issue is needed since proposal in CB-12397 was rejected in apache/cordova-discuss#69 (comment).

[brodybits]

Closing in favor of the solution proposed by @raphinesse in apache/cordova-discuss#69 (comment).

[brodybits]

As I said in apache/cordova-app-hello-world#22 (comment) I think we still need a workaround solution like this one due to NPM behavior, reopening for discussion.

I am about to take a long weekend, will be back next Tuesday.

P.S. I would be happy to rebase and retest sometime next week.

[brodybits]

I just pushed WIP update to this branch, based on some spec updates from #14, with XXX TODO comment that the final change is waiting for #14 to be merged.

Remaining TODO items that I can think of:

• resolve item marked XXX TODO in changes to index.js
• use _gitignore instead of .npmignore as requested in [CB-12397] .gitignore instead of unwanted .npmignore after cordova create (7.0.1) cordova-discuss#69 (comment)
• spec should probably to be updated to check that _gitignore is copied correctly to .gitignore and no extra _gitignore artifact is present

[brodybits]

Looking at the code I realized that it will be harder than I had thought before to get this one right. The existing code in index.js seems to overwrite some but not all artifacts from cordova-app-hello-world with artifacts from the optional user-provided template. There is also some difference in handling of different artifacts depending on whether or not there is a template subdirectory.

I suspect that this would already be documented to a certain extent, not sure how complete or accurate it really is. I would like to see the documentation updated as well.

Unfortunately I do not have the time to review the existing behavior and documentation due to a critical client deadline, should be able to revisit in 1-2 months or so. My bad for not seeing this before working on #14.

Another thing is that if we switch over to using _gitignore in cordova-app-hello-world and user templates this would likely need a major release. We may have a better chance of fixing this issue in a minor release if we continue with a hack such as:

• use .npmignore from cordova-app-hello-world for generated .gitignore if no .gitignore or .npmignore is present in the user-provided template (or there is no user-provided template)
• in case of user-provided template with .gitignore file we use that one
• we document that we support .gitignore but NOT .npmignore in user-provided template
• test to see if we have to make a hack in case NPM renames .gitignore in user-provided template (maybe this was already done by @raphinesse)

I will understand if the .gitignore problem proves too unwieldy and we have to use _gitignore instead (and document it), would still like to avoid the whole _gitignore thing if possible.

I will understand if someone else wants to take this one over, would hope to see it done with the kind of testing I had already done for this one and #14.

[raphinesse]

Yes, the current logic is pretty convoluted. Most of it is not documented either, AFAIK. I proposed to simplify much of it in apache/cordova-discuss#89 which also outlines some of the current logic.

For the reasons mentioned in apache/cordova-discuss#69 I do not support the "hacky" solution proposed by you. I rather hope that we can land some changes from apache/cordova-discuss#89 and build an easy and clean solution on top of the simplified cordova-create base.

And I think #14 is still fine, even if this change here is delayed a bit.

[brodybits]

Leaving it up to @raphinesse or anyone else who wants to continue working on this fix. I would be happy to answer any questions or contribute feedback on GitHub. I HIGHLY recommend using the test updates I contributed in #14 (also included in this PR) as a starting point.

[brodybits]

Reopening for discussion. I would be happy to revisit this fix when I get some time, with the _gitignore workaround as proposed before. I will probably need 1-2 months to deliver a critical fix to a customer before I can take another look, still wouldn't mind if someone else wants to jump in and take this one over.

I continue to be very concerned about what seems like inconsistent behavior about which artifacts do and do not get overwritten from the user-provided template, if provided, which artifacts still come from cordova-app-hello-world if the user-provided template is present, which artifacts are or are not taken from subdirectory if present, etc. I remain extremely hopeful that this inconsistency will be resolved and actual behavior better documented.

In addition I would like to see the spec updates I proposed in f4b752d (part of #14) merged ASAP, will raise in a new PR.

[raphinesse]

@brodybits please don't open a new PR for the test changes. I'm just now preparing a PR that builds on top of your changes. I think I might have it ready later today.

[brodybits]

@raphinesse got it. I hope it will check for presence vs absence of both .gitignore and .npmignore, like I had proposed in f4b752d.

Rationale: there are cases reproduced in f4b752d where cordova-create does create unwanted .npmignore file, due to the way cordova-create uses NPM to consume the cordova-app-hello-world dependency. I think we want the actual behavior to be known today and verified fixed for good in the future.