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

Ovis 4.4.2 #1325

Closed
wants to merge 0 commits into from
Closed

Ovis 4.4.2 #1325

wants to merge 0 commits into from

Conversation

oceandlr
Copy link
Member

Updated pull request, separating out fixes to configure.ac and slingshot_switch sampler.

configure.ac Outdated
@@ -927,6 +927,12 @@ AS_IF([test "x$enable_slingshot" = xyes],[
[AC_MSG_ERROR([libcxi or its headers not found])])
])

AC_ARG_ENABLE([slingshot_switch],
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I can tell, there are no external software dependencies in this sampler. So maybe we don't need to make compilation conditional?

Copy link
Member Author

Choose a reason for hiding this comment

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

@valleydlr We want the ability to optionally not build for those wanting to explicitly limit the build

configure.ac Outdated
AC_ARG_ENABLE([slingshot_switch],
[AS_HELP_STRING([--enable-slingshot_switch], [require the slinghost on-switch plugins @<:@default=check@:>@])],
[],
[enable_slingshot_switch="check"])
Copy link
Collaborator

@morrone morrone Jan 17, 2024

Choose a reason for hiding this comment

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

While it won't actually change the behavior, it would be better to have this say enable_slingshot_switch="yes", because there is no actually checking of dependencies. If build is not explicitly disabled it will attempt to build it. (and change s/check/yes in the comment as well)

configure.ac Outdated
@@ -246,6 +246,7 @@ OPTION_DEFAULT_DISABLE([yaml], [ENABLE_YAML])

dnl Options for store
OPTION_DEFAULT_ENABLE([store], [ENABLE_STORE])
OPTION_DEFAULT_ENABLE([decomp], [ENABLE_DECOMP])
Copy link
Collaborator

Choose a reason for hiding this comment

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

In a previous review, you said that we want to be able to disable decomp build when there are no stores. So could this just use the ENABLE_STORE option? Does it really need its own?

Copy link
Contributor

Choose a reason for hiding this comment

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

Added this to support a minimum build that could be installed on a network switch (slingshot switch in this case).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am sorry; I am not following. Are you saying that on network switches, you need to enable stores, but disable decomp?

If you want to disable both stores and decomp at the same time, then decomp could be disabled with the "ENABLE_STORE" variable, and wouldn't need its own.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. You are suggesting changing the ENABLE store option for both. Will change and resubmit:)

@morrone
Copy link
Collaborator

morrone commented Jan 20, 2024

@oceandlr Could you please update this PR with a title that reflects the content of the changes?

@tom95858
Copy link
Collaborator

@oceandlr is everyone ready to merge this? It does need to be rebased. I think @morrone would like the description to be changed to "slingshot switch monitoring software" or something like that.

@tom95858
Copy link
Collaborator

@oceandlr, I think all we need to do here is rebase (to get rid of conflicts) and address @morrone's request to change the pull request description.

configure.ac Outdated
@@ -468,7 +468,8 @@ AC_ARG_ENABLE([ibnet],
[],
[enable_ibnet="check"])
AM_CONDITIONAL([ENABLE_IBNET], [test "x$enable_ibnet" != xno])
AS_IF([test "x$enable_ibnet" = xyes],[
AS_IF([test "$enable_ibnet" = xyes],[
Copy link
Collaborator

Choose a reason for hiding this comment

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

That change is not correct.

configure.ac Outdated
@@ -468,7 +468,8 @@ AC_ARG_ENABLE([ibnet],
[],
[enable_ibnet="check"])
AM_CONDITIONAL([ENABLE_IBNET], [test "x$enable_ibnet" != xno])
AS_IF([test "x$enable_ibnet" = xyes],[
AS_IF([test "$enable_ibnet" = xyes],[
AC_MSG_NOTICE([Disable ibnet module NOT requested])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure that his notice makes sense to add.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@morrone, no it's broken. I was playing with the github editor and accidentally broke it. the 'x' is missing, etc... @valleydlr has changes staged that he will test today and push tomorrow.

@morrone
Copy link
Collaborator

morrone commented Feb 14, 2024

I rebased this and cleaned it up over in #1361.

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