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

Remove irrelevant hookcode, fix ti clean forking with --project-dir arg #402

Merged
merged 5 commits into from Oct 20, 2020

Conversation

ewanharris
Copy link
Contributor

  • Removes a bunch of compatibility fixes for pre 4.X SDKs that are no longer required
  • Fixes support for forking when running ti clean --project-dir <dir> when the selected SDK and tiapp SDK differ (TIMOB-27866), and renames the hook file to be representative of it's new purpose

@build
Copy link

build commented Oct 19, 2020

Messages
📖

✅ All tests are passing
Nice one! All 100 tests are passing.

Generated by 🚫 dangerJS against 6a8c59f

hooks/sdk-fork-fix.js Outdated Show resolved Hide resolved
hooks/sdk-fork-fix.js Outdated Show resolved Hide resolved
hooks/sdk-fork-fix.js Outdated Show resolved Hide resolved
hooks/sdk-fork-fix.js Outdated Show resolved Hide resolved
hooks/sdk-fork-fix.js Outdated Show resolved Hide resolved
hooks/sdk-fork-fix.js Outdated Show resolved Hide resolved
tiPath = path.join(cli.sdk.path, 'node_modules', 'node-titanium-sdk', 'lib', 'titanium.js');

if (!fs.existsSync(tiPath)) {
tiPath = path.join(cli.sdk.path, 'node_modules', 'titanium-sdk', 'lib', 'titanium.js');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need this line because according to https://wiki.appcelerator.org/display/guides2/Titanium+Compatibility+Matrix#TitaniumCompatibilityMatrix-SupportedSDKreleases, we still support SDK 6.0.x and 6.1.x... which are over 3 years old! I can't believe we still support SDK 6.x. I think we need someone to double check our supported SDK releases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should remove that statement from the docs, we shouldn't be putting supported versions on random pages. Maybe it should link to the product lifecycle doc instead (although it requires some mental gymnastics to try and figure out the valid versions from there :)

hooks/sdk-fork-fix.js Outdated Show resolved Hide resolved
@cb1kenobi
Copy link
Contributor

At a high level, I'm cool with this PR, but I think we should probably just fix the ti.validateCorrectSDK() call in the SDK, then we can remove this hook in a couple years... even though this won't even be a problem in Titanium CLI.next.

…on check

node-titanium-sdk is in all supported SDK lines so we can trust that it will be there, this
check doesn't need to compare an SDK version so we don't need to make sure that we can get
a semver compliant SDK version
@ewanharris
Copy link
Contributor Author

ewanharris commented Oct 19, 2020

@sgtcoolguy, @cb1kenobi, pushed an update to do the following:

  • remove titanium-sdk lookup
  • use const/let
  • remove the getSDK check
  • move and negate the pd check up to return early

I still need to do the following:

  • remove the supported SDKs from the wiki
  • investigate the winston 2.x upgrade, I believe it has broken logging

@tidev tidev deleted a comment from sgtcoolguy Oct 19, 2020
Copy link
Contributor

@cb1kenobi cb1kenobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found an issue. The issue exists with the current Titanium CLI code. Here's the scenario:

  1. App is set to Titanium SDK 9.0.0.GA
  2. Titanium CLI has 9.0.3.GA as the selected SDK
  3. Make sure you are NOT in a Titanium app directory (like your home directory)
  4. Run titanium build with NO additional options/args to trigger prompting
  5. Titanium CLI loads SDK 9.0.3.GA and does validation phase
  6. Titanium CLI prompts you for the platform
  7. Titanium CLI prompts for the project directory using the --project-dir prompt "field" from SDK 9.0.3.GA
  8. Type a valid path to a Titanium app
  9. Project dir prompt takes input and passes it to the field's validate callback, which is really just the project dir option's validate that is monkey patched by sdk-fork-fix.js
  10. pd.validate() is called, which in turn calls origValidate()... ti.validateCorrectSDK() is a noop, so origValidate() succeeds
  11. realValidateCorrectSDK() is called... 9.0.3.GA != 9.0.0.GA... forks correct Titanium CLI command and throws GracefulShutdown
  12. Error caught by fields, prints empty [ERROR], reprompts for project directory using 9.0.3.GA field
  13. Correct Titanium CLI with 9.0.0.GA selected fires up, but doesn't know the project dir you just entered... prompts for project dir using 9.0.0.GA
  14. You type the project dir again... but now you have TWO prompts prompting at the same time!!! WTF?
  15. Typing the path is useless because the prompts stomp on each other's state... you must paste the path and hit enter QUICKLY (may take a few times)
  16. Build finally takes off, app builds fine, sim launches fine
  17. Everyone is completely confused

As stated above, this happens before this PR, but it seems like something that might be able to be fixed here.

The problem is project dir's validate() doesn't know if it's being run from the Titanium CLI when --project-dir is specified or . resolves a validate dir or if it was prompting. Fields happily prints the error and reprompts.

So, to fix this, you need to monkey patch project-dir's prompt() to monkey patch field's prompt() just so you can intercept the GracefulShutdown error! Wowza! Here's the code:

		const origPrompt = pd.prompt;
		const origValidate = pd.validate;
		let gracefulShutdown = false;

		pd.validate = function (projectDir, callback) {
			return origValidate(projectDir, function (err, projectDir) {
				if (!err) {
					// if we don't have a tiapp loaded, then the --project-dir callback() wasn't
					// called, so just call it now
					if (!cli.tiapp) {
						projectDir = pd.callback(projectDir);
					}

					// force overwrite when validate() was called during prompting, otherwise
					// the value should be the same
					cli.argv['project-dir'] = projectDir;

					// now validate the sdk
					if (!realValidateCorrectSDK(logger, config, cli, commandName)) {
						throw new cli.GracefulShutdown();
					}
				}
				callback(err, projectDir);
			});
		};

		pd.prompt = callback => {
			origPrompt(field => {
				field.validate = (projectDir, cb) => {
					try {
						pd.validate(projectDir, cb);
					} catch (err) {
						if (err instanceof cli.GracefulShutdown) {
							gracefulShutdown = true;
							return true;
						} else {
							cb(err);
						}
					}
				};
				const origFieldPrompt = field.prompt.bind(field);
				field.prompt = cb => {
					origFieldPrompt((err, value) => {
						if (err) {
							cb(err);
						} else if (gracefulShutdown) {
							// do nothing!
						} else {
							cb(null, value);
						}
					});
				};
				callback(field);
			});
		};

This [I think] will fix all possible avenues:

  1. titanium build outside the app directory with prompting
  2. titanium build --project-dir <path> outside the app directory (no prompting)
  3. titanium build inside an app directory (no prompting)

Note that the prompting of project-dir is the only relevant prompt here. Any other prompt (like pp-uuid, output-dir, etc) is unaffected since they don't throw a GracefulShutdown error.

@ewanharris
Copy link
Contributor Author

@cb1kenobi, I added code to force a file prompt for the --project-dir on a ti clean if one doesn't exist. Also created tidev/titanium-sdk/pull/12194 to actually set a prompt rather than using the default

Copy link
Contributor

@cb1kenobi cb1kenobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't have written it any better if I did it myself. APPROVED!

@sgtcoolguy sgtcoolguy merged commit 719def7 into tidev:master Oct 20, 2020
@ewanharris ewanharris deleted the remove_hookcode branch February 11, 2021 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants