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

plugins/spectrum_analyzer: Add cmake option to build it or not #352

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dNechita
Copy link
Contributor

And by default exclude it from build.

Signed-off-by: Dan Nechita [email protected]

Copy link
Contributor

@mhennerich mhennerich left a comment

Choose a reason for hiding this comment

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

Wondering, if we're not building and installing the plugin anymore how can the ifdef in identify() do anything?
The original problem I see is that there is no clean uninstall when a new OSC is build and installed.
So all residual plugin SOs will get loaded. the only way to prevent this is to clean the plugin folder. Or to update the plugin with this identify hack.

@dNechita
Copy link
Contributor Author

Wondering, if we're not building and installing the plugin anymore how can the ifdef in identify() do anything?
The original problem I see is that there is no clean uninstall when a new OSC is build and installed.
So all residual plugin SOs will get loaded. the only way to prevent this is to clean the plugin folder. Or to update the plugin with this identify hack.

Makes sense. Initially I kept building the plugin but then I modified. I will make the changes.

And by default exclude it from build.

Signed-off-by: Dan Nechita <[email protected]>
@dNechita dNechita force-pushed the optionally-enable-spectrum-plugin branch from 843a56b to 4fdba24 Compare August 11, 2021 10:46
add_definitions(-DPLUGIN_SPECTRUM_ANALYZER)
endif()


Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this option should be in the plugins specific CMakeLists file. Moreover, I would just not compile the library at all if the option is OFF. As it is the default value, I would remove the plugin from the list of plugins and just add it if (WITH_PLUGIN_SPECTRUM_ANALYZER)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that initially but as Michael said we need to build the plugin. As for the option, I prefer all options to be at top level cmake.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cannot say I agree with that but fair enough...

@nunojsa
Copy link
Contributor

nunojsa commented Aug 11, 2021

Wondering, if we're not building and installing the plugin anymore how can the ifdef in identify() do anything?
The original problem I see is that there is no clean uninstall when a new OSC is build and installed.
So all residual plugin SOs will get loaded. the only way to prevent this is to clean the plugin folder. Or to update the plugin with this identify hack.

IMO, I would just vote for the first option "clean the plugin folder"... Maybe adding an uninstall target?

tfcollins
tfcollins previously approved these changes Aug 13, 2021
@SRaus SRaus changed the base branch from master to main January 9, 2024 09:42
@SRaus SRaus dismissed tfcollins’s stale review January 9, 2024 09:42

The base branch was changed.

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