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.

Sign up for GitHub

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

Jump to bottom

Correctly format include paths for eclipse export #3531

Merged
merged 2 commits into from
Jan 9, 2017
Merged

Correctly format include paths for eclipse export #3531

merged 2 commits into from
Jan 9, 2017

Conversation

bittailor
Copy link
Contributor

@bittailor bittailor commented Jan 5, 2017 •
edited by theotherjimmy
Loading

Eclipse CDT expects the include paths to include the project name like '//' for workspace include directories.
See issue #3529.

Status

READY

Migrations

NO

Related PRs

Todos

  • Tests

Deploy notes

Steps to test or reproduce

See issue #3529

@bittailor
Correctly format include paths for eclipse export
b668185 
Eclipse CDT expects the include paths to include the project name like '/<project>/<include-path>' for workspace include directories.
See issue ARMmbed#3529.
@bittailor bittailor mentioned this pull request Jan 5, 2017
Closed
theotherjimmy
theotherjimmy suggested changes Jan 5, 2017
View reviewed changes
tools/export/cdt/__init__.py
@@ -12,13 +14,14 @@ def generate(self):
py_ocd_settings launch file, and software link .p2f file
"""
super(Eclipse, self).generate()
include_paths_replace_re= re.compile(r'(^[.]/|^[.]$)')
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this variable have a shorter name? maybe starting_dot? which implies that this is the same as <string>.startswith(".")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

tools/export/cdt/__init__.py
ctx = {
'name': self.project_name,
'elf_location': join('BUILD',self.project_name)+'.elf',
'c_symbols': self.toolchain.get_symbols(),
'asm_symbols': self.toolchain.get_symbols(True),
'target': self.target,
'include_paths': self.resources.inc_dirs,
'include_paths': map(lambda s: include_paths_replace_re.sub('%s/' % self.project_name, s), self.resources.inc_dirs),
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with the rest of the tools, could this be a comprehension?

'include_paths': [starrting_dot.sub('%s/' % self.project_name, inc) for inc in self.resources.inc_dirs],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
I'm a python newbie did not know about comprehension.
Thanks

@theotherjimmy
Copy link
Contributor

I like where this is going!

Did you test it manually? is that why you checked the "Tests" Todo?

@bittailor
Fix review requests
75a61df 
- Shorten variable name
- Use comprehension to format include paths
@bittailor
Copy link
Contributor Author

bittailor commented Jan 5, 2017 •
edited
Loading

@theotherjimmy Yes I just did a manual test on my machine and did not add a unit test for it. Was probably to quick to check "Tests" Todo.

I'm new to python and new to mbed-os, could you point me to the place where I should add a unit test and how to run then.

I tried to run:

$ PYTHONPATH=. python tools/test/export/build_test.py

But this failed with:

1/144 Exporting test case  KW41Z::iar	tests-mbedmicro-rtos-mbed-basic
2/144 Exporting test case  KW41Z::uvision	tests-mbedmicro-rtos-mbed-basic
Traceback (most recent call last):
  File "tools/test/export/build_test.py", line 298, in <module>
    main()
  File "tools/test/export/build_test.py", line 293, in main
    test.batch_tests(clean=options.clean)
  File "tools/test/export/build_test.py", line 116, in batch_tests
    do_queue(Reader, self.perform_exports, self.tests)
  File "tools/test/export/build_test.py", line 57, in do_queue
    each.start()
  File "tools/test/export/build_test.py", line 71, in start
    self.func(test)
  File "tools/test/export/build_test.py", line 173, in perform_exports
    silent=True, build_profile=profile)
TypeError: export() got an unexpected keyword argument 'clean'

@theotherjimmy
Copy link
Contributor

Sorry about that @bittailor #3538 has the fix.

@theotherjimmy
Copy link
Contributor

theotherjimmy commented Jan 5, 2017 •
edited
Loading

For this PR, a manual check that it looks correct on your end will do. I don't know how we would automate that check at the moment. Suggestions welcome!

Because you said that you have already done the manual check, I checked the "Tests" todo for you.

@sg- sg- merged commit 613b92a into ARMmbed:master Jan 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theotherjimmy theotherjimmy theotherjimmy approved these changes

Assignees
No one assigned
Labels
release-version: 5.3.3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bittailor @theotherjimmy @sg-