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

[TIMOB-23247] CLI: Implement Native Module clean #9807

Merged
merged 7 commits into from May 1, 2018

Conversation

sgtcoolguy
Copy link
Contributor

JIRA: https://jira.appcelerator.org/browse/TIMOB-23247

Description:

 ti clean -p <platform>

This hooks up ti clean as a command that can be used for modules as we do with normal apps.
It's implemented in a single-platform at a time way, and isn't meant to yet support multi-platform modules like say hyperloop.

I took a best guess at the files we should be wiping out, but certainly open to suggestions/tweaks here.

@build
Copy link
Contributor

build commented Feb 6, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

// TODO: Use a glob and delete any version zipfile generated?
const moduleVersion = cli.manifest.version;
const moduleZipName = [ moduleId, '-iphone-', moduleVersion, '.zip' ].join('');
const toDelete = [ 'build', moduleAssetsFile, moduleZipName ];
Copy link
Collaborator

Choose a reason for hiding this comment

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

metadata.json

exports.run = function run(logger, config, cli, finished) {
const projectDir = cli.argv['project-dir'];

const toDelete = [ 'build', 'dist', 'libs', 'java-sources.txt' ];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the build command already handles the recreation, but "build" has to exist and "libs" has to include the used architectures, e.g arm64-v8a. Also, many devs curate a list of archives in the dist directory that will be flushed out now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Android goes a little nutty if libs already exists, sometimes. I think it tries to do an architecture sanity check, but I'm not sure why because that's an output directory for the static libraries it generates during he build.

I tested cleaning and building a couple native modules, and it handles recreating the build dir.

As far as dist goes, I get that some devs keep the generated zips there and check them into SCM, but that's not exactly a great practice. The zips should be published to S3 or Github releases or something, not checked into source.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd still rather not want to break this in a minor version and don't see the advantage of deleting them before, as they do not cause any issues right now, different to the other cleaned directories. Maybe move that change to 8.0.0 instead.

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 struggle to see how this is a breaking change. If a developer has a not-so-great practice of checking in generated zips to their SCM and the new module clean command will delete that directory, the file will still be in their SCM. This command will not "checkin" the deletion of the zip. And even if the user did check the deletion into SCM, the file would still be in their SCM history to restore.

The dist folder is generated output folder that should be cleaned before a clean build.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I only see the following JIRA ticket incoming:

"I create super duper nice module packages since 6 years and now I lost all my clients because the module zips are gone. Why do you do that?"

If we can defend that with being a best practice, let's do it. Not trying to block this.

Copy link
Collaborator

@hansemannn hansemannn left a comment

Choose a reason for hiding this comment

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

Some outstanding review changes.

@sgtcoolguy sgtcoolguy modified the milestones: 7.1.0, 7.2.0 Feb 15, 2018
@sgtcoolguy sgtcoolguy merged commit 265dc48 into tidev:master May 1, 2018
@sgtcoolguy sgtcoolguy deleted the module-clean branch May 1, 2018 13:39
@hansemannn
Copy link
Collaborator

Don‘t we need QE testing here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants