fix(build): support tilde expansion on windows

[hiepxanh]

Platforms affected

Android, ios, Browser

What does this PR do?

I struggeling with home path every time changing build computer or transfer code to my teammate computer. the build.json config does not find home path on window with "command line" or "power shell". there is no tutorial about this. I have to go to build file to know that there is ~ equivalent with $HOME and %HOMEPATH% on window.
I think it is feature?

What testing has been done on this change?

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.

[janpio]

Code is currently broken

[janpio]

Hm, first time I see this. Is the ~ behavior documented anywhere? Where did you learn that you can do it this way?

Also: Isn't the code here currently already exactly for making it possible to use ~ on Windows, where this isn't supported out of the box?

[hiepxanh]

Hm, first time I see this. Is the ~ behavior documented anywhere? Where did you learn that you can do it this way?

=> It take me to 5 hours to learn that there is no documented or any suggestion from google ( I was read about 10 google pages <=> 50 web page with a lot of try times) but it have nothing. Then, I go to read the source code. The document is lack of this.
The situation when you have a team with alot of computer, you have different home path.

Also: Isn't the code here currently already exactly for making it possible to use ~ on Windows, where this isn't supported out of the box?

=> yes it support windows. that very nice, except:
not every body install $HOME enviroment variable as home path. Only after I read the source code. I found out that I have to use ~ and it match to variable $HOME, but I dont have that path ??. Then I have to update my window enviroment variable with this path.

that was take a lot of time.

[codecov-io]

Codecov Report

Merging #563 into master will decrease coverage by 0.18%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #563      +/-   ##
==========================================
- Coverage   62.11%   61.92%   -0.19%     
==========================================
  Files          17       17              
  Lines        1985     1991       +6     
  Branches      371      374       +3     
==========================================
  Hits         1233     1233              
- Misses        752      758       +6

Impacted Files	Coverage Δ
bin/templates/cordova/lib/build.js	27.2% <0%> (-1.26%)	⬇️

---

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 f1396c7...a2ca994. Read the comment docs.

[janpio]

So your issue is that you develop your app on different computers. To be able to use the same build.json on all machines, you used ~ in the path to the keystore - but this didn't work as HOME was not correctly set on some machines. Is that correct?

And to "fix" that, you now implemented this PR that allows the usage of $HOME, %HOMEPATH% and %USERPROFILE% instead of only ~ in the path of the keystore. But doesn't this still mean that all computer using the build.json still have to have exactly one of those environment variables set to work?

Wouldn't it make more sense to try to replace ~ with all those environment variables, whichever are available instead of just offering alternatives?

Is the usage of ~ in paths properly documented somewhere?

[janpio]

After writing this, I decided to look into why this tilde code is there in the first place:
d78ae30
https://issues.apache.org/jira/browse/CB-10105
Maybe this is not needed any more in the first place as the problem in node was fixed?

Your original issue nd problem seems more like an unintended side effect of this "hack" that was used to work around this node problem.

Anyway, the usage of environment variables in the path itself is a totally new and different feature and I am not sure this is the way to go.

[brodybits]

Thanks @hiepxanh for the proposal, my response is in apache/cordova-cli#359 (comment).

[raphinesse]

@janpio I'm not aware that the lack of tilde expansion has ever been accepted as being a bug in Node.js. AFAIK, it had always been like that and there are a variety of libraries for this use-case. I prefer https://www.npmjs.com/package/untildify

[janpio]

Thanks @raphinesse for this necessary context.

So if we decide to keep this functionality, it would probably make sense to switch over to using this package (any maybe make this available to all platforms along the way?). The package uses os.homedir() internally, which is probably a lot more stable than any hack we can place in our code ourselves. Agree?

[brodybits]

So if we decide to keep this functionality, it would probably make sense to switch over to using this package (any maybe make this available to all platforms along the way?).

Agreed on my part and I would definitely favor making this available to all platforms. (I was about to suggest the same idea.)

fixed

[janpio]

suggested changes don't actually solve the underlying problem and create more unrelated problems

[brodybits]

suggested changes don't actually solve the underlying problem [...]

I think that means changes suggested 2 days ago and not the recent suggestion to use untildify.

[janpio]

@brodybits Pull Request reviews relate to the code being suggested in the Pull Request, so of course yes, this does not refer to the comments and discussion made here - I just made sure it is clear that the suggested code should not be merged by anyone.

[hiepxanh]

@janpio @brodybits thank you for reviewing my small idea. I think this will be helpful and help alot of people like me. Have to build code with team have different develop enviroment. I agree that my solution not work with all platforms and not a global solution. So I think that we should use @raphinesse suggested library. It will be nice.
thank you for helping me. Should I try to use that library in my code then you can continue review or you will help me to do that and I will close this issue?

[brodybits]

@hiepxanh it would be great if you could try using untildify to solve the problem. Let's keep your proposals open for now. @janpio entered reviews with "changes requested" on all your proposals to tell the other committers not to merge them prematurely.

[hiepxanh]

@brodybits yes sir. I will do it

Outdated

[codecov-commenter]

Codecov Report

Merging #563 (09a870a) into master (16ff6e1) will increase coverage by 0.06%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #563      +/-   ##
==========================================
+ Coverage   73.20%   73.26%   +0.06%     
==========================================
  Files          21       21              
  Lines        1646     1646              
==========================================
+ Hits         1205     1206       +1     
+ Misses        441      440       -1     

Impacted Files	Coverage Δ
lib/build.js	40.59% <50.00%> (+0.99%)	⬆️

---

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 16ff6e1...09a870a. Read the comment docs.

[raphinesse]

I redid the fix using untildify