search

keymanapp / keyman

Keyman cross platform input methods system running on Android, iOS, Linux, macOS, Windows and mobile and desktop web
https://keyman.com/
Other
372 stars 102 forks source link

chore(common): maintenance on build scripts - cd #11329

Closed mcdurdin closed 2 weeks ago

mcdurdin commented 2 weeks ago

Fixes #11324.

@keymanapp-test-bot skip

keymanapp-test-bot[bot] commented 2 weeks ago

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

mcdurdin commented 2 weeks ago

Okay, one problem here is that build-utils.sh is used for both builder scripts and other utility scripts, some of which are supposed to not change folder (e.g. mkver.inc.sh).

Two ways to fix:

rc-swag commented 2 weeks ago

Okay, one problem here is that build-utils.sh is used for both builder scripts and other utility scripts, some of which are supposed to not change folder (e.g. mkver.inc.sh).

Two ways to fix:

It took me a while to even understand the problem you are solving. Let me just put it in my words to see if it makes sense. If you add the line cd "$THIS_SCRIPT_PATH" to builder.inc.sh. builder.inc.sh is sourced in build-utils.inc.sh. So now all the subprojects that are sourcing build-utils.inc.sh will have the directory changed to the script path pf the calling script. This is not desirable eg mkver.inc.sh in which you want it to be the directory it is currently in at the time you source build-utils.inc.sh . I would go with BUILDER_NO_CD flag. That is because adding another script just to not do a cd seems confusing. What about If $THIS_SCRIPT_PATH was called $EXECUTE_PATH then for those scripts like that do not want to change directory set the $EXECUTE_PATH to pwd directory.

mcdurdin commented 2 weeks ago

Let me just put it in my words to see if it makes sense. If you add the line cd "$THIS_SCRIPT_PATH" to builder.inc.sh. builder.inc.sh is sourced in build-utils.inc.sh. So now all the subprojects that are sourcing build-utils.inc.sh will have the directory changed to the script path pf the calling script. This is not desirable eg mkver.inc.sh in which you want it to be the directory it is currently in at the time you source build-utils.inc.sh .

Yep.

I would go with BUILDER_NO_CD flag. That is because adding another script just to not do a cd seems confusing. What about If $THIS_SCRIPT_PATH was called $EXECUTE_PATH then for those scripts like that do not want to change directory set the $EXECUTE_PATH to pwd directory.

I kinda started to go this way then realised I'd need to fixup all the random scripts that include build-utils.sh to do this, and it would still leave us in the spot where api contract for builder scripts is vague. So I ended up:

  1. Creating resources/build/builder.inc.sh which imports build-utils.sh and also does the cd
  2. Updating all builder-compliant scripts to use that one
  3. Remaining scripts continue to use build-utils.sh
  4. resources/builder.inc.sh may be renamed in future to avoid confusion
rc-swag commented 2 weeks ago

4. resources/builder.inc.sh may be renamed in future to avoid confusion

Yes I think that needs to happen, something that helps reflect what it does the resources/build/builder.inc.sh doesn't. Actually it is really the other way around isn't?. the later is an extension

mcdurdin commented 2 weeks ago
  1. resources/builder.inc.sh may be renamed in future to avoid confusion

Yes I think that needs to happen, something that helps reflect what it does the resources/build/builder.inc.sh doesn't. Actually it is really the other way around isn't?. the later is an extension

I think resources/builder.inc.sh should become resources/build/builder.implementation.inc.sh or something like that (better not to have it in resources/ directly).

keyman-server commented 2 weeks ago

Changes in this pull request will be available for download in Keyman version 18.0.28-alpha