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

TFLu: Fix Ethos-U build issue #42823

Merged
merged 3 commits into from Oct 15, 2020

Conversation

mansnils
Copy link
Contributor

The issue is that a binary need to be built twice, since it
depends on recursive_find. The driver is not fully downloaded when
recursive_find is called.
The solution proposal is to call the download script
immediately via the make's shell function.

Also add missing exit to error case in download script.

The issue is that a binary need to be built twice, since it
depends on recursive_find. The driver is not fully downloaded when
recursive_find is called. The solution is to call the download script
immediately via the make's shell function.

Also add missing exit to error case in download script.
@google-ml-butler google-ml-butler bot added the size:S CL Change Size: Small label Aug 31, 2020
@gbaned gbaned self-assigned this Aug 31, 2020
@gbaned gbaned added comp:lite TF Lite related issues comp:micro Related to TensorFlow Lite Microcontrollers labels Aug 31, 2020
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Aug 31, 2020
@gbaned gbaned requested a review from advaitjain August 31, 2020 13:03
@gbaned
Copy link
Contributor

gbaned commented Aug 31, 2020

Thanks for contributing to TensorFlow Lite Micro.

To keep this process moving along, we'd like to make sure that you have completed the items on this list:
Read the contributing guidelines for TensorFlow Lite Micro
Created a TF Lite Micro Github issue
Linked to the issue from the PR description

We would like to have a discussion on the Github issue first to determine the best path forward, and then proceed to the PR review.

@advaitjain
Copy link
Member

I'll let @petewarden review this since he is most familiar with the Makefile logic and its repercussions.

@gbaned gbaned added the awaiting review Pull request awaiting review label Sep 2, 2020
@@ -1,19 +1,31 @@
ifneq ($(filter ethos-u,$(ALL_TAGS)),)
# Don't want -lm flag
MICROLITE_LIBS :=
MICROLITE_LIBS := $(filter-out -lm,$(MICROLITE_LIBS))
Copy link
Contributor

Choose a reason for hiding this comment

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

Add short comment to why we filter out the -lm flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like it is not longer needed, will remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mansnils Any update on this PR? Please. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might still be needed. So I am adding a comment. I think it more belong in a tools/make/target/makefile but there is none for this yet so let's keep it here for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant, what happens if we use the -lm flag? What issue does it cause?

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

Copy link
Member

Choose a reason for hiding this comment

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

Will #43726 make the filtering out here not be needed anymore? Let's get this PR merged since its been sitting for a long time but then circle back and have the exclusion in a single place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as the target in #43726 is used I guess the filtering out here is not needed anymore. As said let's revisit this later.

@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Sep 26, 2020
@gbaned gbaned requested review from petewarden and removed request for petewarden October 5, 2020 13:51
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Oct 5, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 5, 2020
@freddan80
Copy link
Contributor

Looks good from my side.

@gbaned gbaned removed the ready to pull PR ready for merge process label Oct 6, 2020
@gbaned gbaned requested review from petewarden and removed request for petewarden October 6, 2020 13:20
@gbaned gbaned added the awaiting review Pull request awaiting review label Oct 9, 2020
@mansnils
Copy link
Contributor Author

@petewarden Ping for review

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Oct 14, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Oct 14, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 14, 2020
@gbaned gbaned removed the awaiting review Pull request awaiting review label Oct 15, 2020
@copybara-service copybara-service bot merged commit cf6a412 into tensorflow:master Oct 15, 2020
PR Queue automation moved this from Approved by Reviewer to Merged Oct 15, 2020
@mansnils mansnils deleted the ethosu_build_issue branch November 17, 2020 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:lite TF Lite related issues comp:micro Related to TensorFlow Lite Microcontrollers ready to pull PR ready for merge process size:S CL Change Size: Small
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

7 participants