Skip to content

Conversation

smuppand
Copy link
Contributor

@smuppand smuppand commented May 13, 2025

Changes

  • Shell scripts now installed to /usr/share/qcom-linux-testkit
  • Fixes for utils/ files installing in incorrect location
  • configure.ac updated to include recursive shell script directories
  • New autogen.sh script added to fully clean and regenerate autotools artifacts

Motivation

Combining C sources and shell scripts under a single build system makes the test kit easier to distribute, install, and maintain across environments.

Test Plan

  • Ran ./autogen.sh, ./configure, make, and make install successfully
  • Verified installed shell script and binary locations
  • Ensured all directories reflect the original repo layout

@mwasilew @craigez

@@ -0,0 +1,131 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why these tests are written in C? It looks like they can be rewritten in shell and there would be no need for compilation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason why these tests are written in C? It looks like they can be rewritten in shell and there would be no need for compilation.

This was initially written for Axiom, and as a short-term enablement, we switched to Lava. It may not be required for now, but we will need it in the long term.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain how it works in axiom? I don't see any service running these programs on bootup or anything similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

setup_systemd.sh will set up the necessary systemd-service to test the reboot functionality. The .c code will also perform some health checks after the service initialization.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see setup_systemd.sh in this PR. Even with that it still doesn't explain why these need to be compiled. There is no API they call that can't be used from shell.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on this, I don't think these C files belong in this repository.

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'm not sure why these can't be included in this repository, especially since they'll be useful when we migrate to Axiom in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can include them when they're useful. For the moment they're not and can be rewritten in shell. Adding dependencies is not a great idea.

Copy link

Choose a reason for hiding this comment

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

If we are not using these files at this time, then let's not include them. Unused code is going to be a burden to someone and confusing for any developers without context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mwasilew @craigez - Removed src and updated. Please review.

@smuppand smuppand force-pushed the main branch 4 times, most recently from ef9dd04 to e5363e9 Compare May 15, 2025 15:26
@smuppand smuppand marked this pull request as draft May 27, 2025 17:51
@smuppand smuppand closed this May 27, 2025
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