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

Make liblttng-ust_sys-sdt.h pass when lttng is not installed #361

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

omajid
Copy link
Member

@omajid omajid commented Aug 17, 2024

The lttng-ust dependency is technically optional. If .NET is installed and that dependency isn't, assume .NET is okay.

The lttng-ust dependency is technically optional. If .NET is installed
and that dependency isn't, assume .NET is okay.
@omajid
Copy link
Member Author

omajid commented Aug 17, 2024

@nicrowe00 What do you think? lttng-ust is an optional dependency. Should we force it to be installed for the sake of this test?

@nicrowe00
Copy link
Contributor

@omajid I'm inclined to say yes, we should force-install it for the test, just to verify that everything is as expected with the dependency. I don't mind though if we decide not to force-install it as it is optional like you mentioned. It depends on how worried we are about optional dependencies.

@tmds
Copy link
Member

tmds commented Sep 16, 2024

The goal of the test is to verify that lttng is working, we shouldn't make it PASS the test when a required dependency is missing.

@omajid I don't know what triggered the PR. If there is an environment where lttng is not present, you can describe it using a trait (like no-lttng), and add it as skipWhen to the test.json. In that environment you can set the trait by using the --trait argument on the test runner.

@omajid
Copy link
Member Author

omajid commented Sep 18, 2024

The goal of the test is to verify that lttng is working

It isn't. The test is trying to verify that lttng-ust has support for systemtap.

Which, now that I think about it, is not something relevant for .NET at all. .NET doesn't care if lttng-ust has some specific feature or not, as long as it works for tracing. And this test doesn't verify whether lttng-ust can trace correctly. So whether this feature is present or not doesn't make a difference to .NET

I am tempted to just delete this test.

If anything, this should be a test in lttng-ust, not .NET.

@tmds
Copy link
Member

tmds commented Sep 18, 2024

I am tempted to just delete this test.

Sounds good to me.

This test doesn't test that .NET is compatible with lttng-ust. It
doesn't test that lttng-ust works for tracing .NET.

This test only looks for features in lttng-ust (specifically, systemtap
support). These features that are not really relevant to .NET. If
anything, this should be a test in the lttng-ust package itself.

To ease maintenance, lets just delete this.
@omajid omajid merged commit 26cdb2d into redhat-developer:main Sep 19, 2024
15 of 19 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.

3 participants