Skip to content

Conversation

@piotrrak
Copy link

Misc fixes for some of the options disabled by configuration options.
This is step in direction of fixing builds with various configuration options configurations.

Self evaluation:

  1. Build test: [ ]Passed [ ]Failed [*]Skipped
  2. Run test: [ ]Passed [ ]Failed [*]Skipped

How to evaluate:

  1. Describe how to evaluate in order to be reproduced by reviewer(s).

meson setup --native-file=disables.ini

disables.ini with contents:

[project options]

enable-blas=false 
enable-tflite-backbone=false 
enable-memory-swap=false 
enable-nnstreamer-backbone=false 
enable-tflite-interpreter=false 
enable-app=false 
ml-api-support='disabled'
enable-test=true
werror=false

@piotrrak piotrrak force-pushed the meson_options_fixes branch from 4b4131a to 129daaf Compare February 21, 2025 10:12
endif

if get_option('platform') != 'android' and host_machine.system() != 'windows'
if get_option('platform') != 'android' and host_machine.system() != 'windows' and get_option('enable-nnstreamer-backbone')
Copy link
Contributor

Choose a reason for hiding this comment

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

This script contains the code if get_option('enable-nnstreamer-backbone') and get_option('platform') != 'android' extra_defines += '-DENABLE_NNSTREAMER_BACKBONE=1' endif.

How about merging this part as well? It seems like it would be fine to move the part extra_defines += '-DENABLE_NNSTREAMER_BACKBONE=1' under this condition, too!

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI) line 407-409

Copy link
Author

Choose a reason for hiding this comment

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

@baek2sm @djeong20 thanks for suggestion, change updated/rebased.
I've dediced to move host_machine.system() != 'windows' there too, that probably won't be necessary in near future.

Copy link
Author

Choose a reason for hiding this comment

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

I've reverted back to not moving subdir('nnstreamer') to the earlier stage of meson.

Copy link
Contributor

@djeong20 djeong20 left a comment

Choose a reason for hiding this comment

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

LGTM!

@skykongkong8
Copy link
Member

According to discussion made from #2930, I believe enable-blas=true should set by default for both x86 and arm situation. Please share your opinion if you find this fix inappropriate!

@piotrrak
Copy link
Author

piotrrak commented Mar 4, 2025

According to discussion made from #2930, I believe enable-blas=true should set by default for both x86 and arm situation. Please share your opinion if you find this fix inappropriate!

The configuration I've picked here is non-default. This configuration was written to make very minimal thing, with as much things as disabled. Is "should" = "we never really shouldn't build with BLAS disabled on x86 or arm"?

Maybe we should consider removing this enable-blas option and make proper choice based on host machine/platform if non-default shouldn't or can't really work - if that is the case.

@skykongkong8
Copy link
Member

skykongkong8 commented Mar 4, 2025

Is "should" = "we never really shouldn't build with BLAS disabled on x86 or arm"?

Yes, I believe that would be more appropriate for the current status since we rely all single-precision computations on cblas (for both x86 and arm). And yes, I think we should really consider removing enable-blas option, but it actually needs more discussion because we might have to introduce multiple backend options per target architecture.

Copy link
Collaborator

@jijoongmoon jijoongmoon left a comment

Choose a reason for hiding this comment

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

LGTM

This fixes build 'enable-nnstream-backbone' meson option
is specified as 'false'

Signed-off-by: Piotr Rak <[email protected]>
This fixes build 'enable-tflite-interpreter' meson option
is specified as 'false'

Signed-off-by: Piotr Rak <[email protected]>
@piotrrak piotrrak force-pushed the meson_options_fixes branch from 8d7f075 to bcbd53e Compare March 10, 2025 10:27
Copy link
Member

@skykongkong8 skykongkong8 left a comment

Choose a reason for hiding this comment

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

LGTM

@jijoongmoon jijoongmoon merged commit cdce8ec into nnstreamer:main Mar 11, 2025
18 checks passed
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.

7 participants