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-8923: Blackberry: Scripts TODO Mac cleanup (part 1) #68

Merged
merged 4 commits into from Jun 6, 2012

Conversation

dlifshitz-maca
Copy link

Reviewers: Alex, JP
Note: only review from c499affa

[Issues Fixed]
TIMOB-8923: Blackberry: Scripts TODO Mac cleanup (part 1)

[Changes]
android/builder.py

  • don't pack blackberry stuff for android

blackberryndk.py

  • removed old TODOs
  • function package
    • copy the non-other-platform files in Resources and overwrite with files from the blackberry folder
  • function __runUnitTests
    • use ip address, if given
    • run the package and deploy unit tests
  • function main
    • add --ip_address for unit tests

blackberry/builder.py

  • removed old TODOs

[Tests]
Test 1: Run unit tests

  1. Run the following command:
    blackberryndk.py -t
  2. Verify all 7 tests pass
  3. Run the following command:
    blackberryndk.py -t --ip_address
  4. Verify all 8 tests pass

@jpl-mac
Copy link

jpl-mac commented May 31, 2012

deploy test fails for me

Command '['blackberry-deploy', '-installApp', '-launchApp', '-device', '192.169.135.131', '-package', '/var/folders/dg/2crkq1ts6r31t90s7wky95g80000gp/T/tmpv14K6j/HelloWorldDisplayMakefile/x86/o-g/HelloWorldDisplayMakefile.bar']' returned non-zero exit status 1 None
Test deploy project to simulator.. FAILED

@dlifshitz-maca
Copy link
Author

Can you enable the logs and post what went wrong?

@jpl-mac
Copy link

jpl-mac commented May 31, 2012


Command '['blackberry-deploy', '-installApp', '-launchApp', '-device', '192.169.135.131', '-package', '/var/folders/dg/2crkq1ts6r31t90s7wky95g80000gp/T/tmpfUqpfQ/HelloWorldDisplayMakefile/x86/o-g/HelloWorldDisplayMakefile.bar']' returned non-zero exit status 1 None
Test deploy project to simulator.. FAILED
    Traceback (most recent call last):
      File "support/blackberry/blackberryndk.py", line 332, in __runUnitTests
    assert 0 == ndk.deploy(ipAddress, barPath)
    AssertionError

Not that it will help much, let me know what more i can enable for debug

@dlifshitz-maca
Copy link
Author

To enable the build log:
blackberryNdk.py, funcrtion _run, change

logfile = os.devnull

to

logfile = None

@jpl-mac
Copy link

jpl-mac commented May 31, 2012

Works now, i had a typo in the ip address :S Shame on me

for filename in files:
fullFilenameSrc = os.path.join(root, filename)
fullFilenameDest = fullFilenameSrc.replace(blackberryResourcesDir, assetsDir, 1)
shutil.copy2(fullFilenameSrc, fullFilenameDest)
Copy link

Choose a reason for hiding this comment

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

This change looks good, my interogation is wether this is consistant with what it done for other platforms. Should blackberry be dumped in the top assets dir or should it be kept as blackberry subfolder? By the same token, should the top directory event be named assets? maybe the name should be the same as for other platforms, otherwise it would need to be documented somewhere.

I think the resources treatment should be the same a on other platforms, so everything feels familiar to the Studio user.

Copy link
Author

Choose a reason for hiding this comment

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

The Ti docs say that Resources/ will be put wherever the stuff in Resources goes and that the other platform folders will be ignored. As for the the name and location of the assets dir I think it depends on the platform. I would have preferred to use "Resources" instead of "assets" but I don't see any benefits that outweigh the extra work.
For documentation for the Studio user I don't see a point since they won't care what's in the bar file. It's our code that needs to know.
http://docs.appcelerator.com/titanium/2.0/index.html#!/guide/Supporting_Multiple_Platforms_in_a_Single_Codebase section "Platform-specific resources"

Copy link

Choose a reason for hiding this comment

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

Thanks David for the information.

So long as the user doesn't need to reference assets in the path to resources then that's all good. In fact judging from the doc you referenced the user should never need to reference the resources folder. We may need to implement that but that is likely outside the scope of this patch.

@jpl-mac
Copy link

jpl-mac commented May 31, 2012

Reviewed

@alexandergalstyan
Copy link

Rather than already posted comments, seems ok. Need to merged with latest version of scripts containing ip_address, password.
Reviewed.

@dlifshitz-maca
Copy link
Author

Updated

@jpl-mac
Copy link

jpl-mac commented Jun 1, 2012

Approved

[Issues Fixed]
TIMOB-8923: Blackberry: Scripts TODO Mac cleanup (part 1)

[Changes]
android/builder.py
- don't pack blackberry stuff for android

blackberryndk.py
- removed old TODOs
- function package
  - copy the non-other-platform files in Resources and overwrite with files from the blackberry folder
- function __runUnitTests
  - use ip address, if given
  - run the package and deploy unit tests
- function __main__
  - add --ip_address for unit tests

blackberry/builder.py
- removed old TODOs

[Tests]
Test 1: Run unit tests
1) Run the following command:
blackberryndk.py -t
2) Verify all 7 tests pass
3) Run the following command:
blackberryndk.py -t --ip_address <simulator ip>
4) Verify all 8 tests pass
@alexandergalstyan
Copy link

Approved

dlifshitz-maca added a commit that referenced this pull request Jun 6, 2012
TIMOB-8923: Blackberry: Scripts TODO Mac cleanup (part 1)
@dlifshitz-maca dlifshitz-maca merged commit c5a8429 into Macadamian:blackberry Jun 6, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants