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

[infra/q-implant] Introduce q-implant Source Config #11535

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

Judgement9882
Copy link
Contributor

for : #11254
draft : #11530

This PR includes a new external library, which is required for q-implant and has a MIT license.

ONE-DCO-1.0-Signed-off-by: Junyeong Kang [email protected]

This PR includes a new external library, which is required for q-implant and has a MIT license.

ONE-DCO-1.0-Signed-off-by: Junyeong Kang <[email protected]>
@Judgement9882
Copy link
Contributor Author

prev PR : #11534

I created a PR before, but I found a problem with my commit message.
So I fixed the commit message and created this PR again.

@Judgement9882
Copy link
Contributor Author

@nnfw-bot test onert-x64-release

@@ -0,0 +1,22 @@
function(_LibnpySource_import)
if(NOT ${DOWNLOAD_LIBNPY})
Copy link
Contributor

Choose a reason for hiding this comment

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

have you tested with DOWNLOAD_LIBNPY OFF ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I set the "DOWNLOAD_LIBNPY" option to "OFF" and proceeded with the test.

As a result, the configuration, build, and test processes of nncc worked without any problems.

Additionally, the test was also conducted when the configuration file of the q-implant module to be added in the future was built together.

In this case, no errors occurred during the entire test process, but this is due to the exception in CMakeLists.txt of q-implant.
(In CMakeLists.txt of q-implant, If LivnpySource_FOUND is false, exit without further building q-implant)
Therefore, the q-implant executable file is not created.

When I change the DOWNLOAD_LIBNPY option to ON and conduct the nncc configure, build process again, q-implant will be built successfully.


ExternalSource_Download(LIBNPY
DIRNAME LIBNPY
URL ${LIBNPY_URL})
Copy link
Contributor

Choose a reason for hiding this comment

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

why are DIRNAME LIBNPY and URL needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Libnpy required for 'q-implant' is an external library source that does not yet exist in ONE. Therefore, download is required.

'DINAME LIBNPY' and 'URL' are used to download external sources from other cmake file.

Therefore, I thought I could refer to it and use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

why I asked is that other lib files didn't seem to have this. plz check othe files and it would be better to follow like oyher filed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I checked and found that it works without using "DINAME LIBNPY" and "URL". Fixed and committed again.

jinevening
jinevening previously approved these changes Sep 18, 2023
Copy link
Contributor

@jinevening jinevening left a comment

Choose a reason for hiding this comment

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

LGTM

@Judgement9882
Copy link
Contributor Author

@nnfw-bot test onert-x64-release

2 similar comments
@0minyoung0
Copy link
Contributor

@nnfw-bot test onert-x64-release

@jinevening
Copy link
Contributor

@nnfw-bot test onert-x64-release

This is a commit fixed to follow the Code Convention in ExternalSource_Download.

ONE-DCO-1.0-Signed-off-by: Junyeong Kang <[email protected]>
@Judgement9882
Copy link
Contributor Author

@nnfw-bot test nncc-jammy-debug

Copy link
Contributor

@jinevening jinevening left a comment

Choose a reason for hiding this comment

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

LGTM

@seanshpark
Copy link
Contributor

LGTM thank you!

@seanshpark seanshpark merged commit 253570d into Samsung:master Sep 18, 2023
3 checks passed
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.

4 participants