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

configure: fix composefs default to on, if appropriate #3071

Closed
wants to merge 1 commit into from

Conversation

ericcurtin
Copy link
Collaborator

composefs should be included by default. If the appropriate headers are
not present, have_mount_attr_idmap should be set to no. If this is no,
don't build composefs as part of the build.

composefs should be included by default. If the appropriate headers are
not present, have_mount_attr_idmap should be set to no. If this is no,
don't build composefs as part of the build.
@openshift-ci
Copy link

openshift-ci bot commented Oct 10, 2023

@ericcurtin: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/fcos-e2e 6811cf3 link true /test fcos-e2e

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@alexlarsson
Copy link
Member

I don't understand how this changes anything? Like we were checking x"$have_mount_attr_idmap" != xyes, and if $have_mount_attr_idmap is unset we should have taken this if, because x != xyes, no?

@ericcurtin
Copy link
Collaborator Author

ericcurtin commented Oct 11, 2023

I suspect have_mount_attr_idmap was never getting set correctly, so composefs_default was always being set as no. Because != yes covers the unset case also. I'm not confident that this is the best fix, this was tested in Fedora, after this change composefs is on because checking for == xno is a more explicit stricter check than != xyes.

But a full test would be to test on an older kernel also, to see if it's no in that case, an explanation of why this conditional stuff was introduced is here:

#2998

@ericcurtin
Copy link
Collaborator Author

ericcurtin commented Oct 11, 2023

The build being red I guess proves composefs is always on with this change. Maybe I can take a look at this when I'm back, although I'm not an autotools expert.

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

I just tested this quickly, running a default build without --with-composefs does enable composefs.

The real problem is in #3067

But I'm fine to land this too, just needs a rebase.

But, this change is perhaps clearer so I'm also fine to land it.

@ericcurtin
Copy link
Collaborator Author

You are right @cgwalters ... Closing...

@ericcurtin ericcurtin closed this Oct 12, 2023
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.

3 participants