Skip to content

Conversation

@3mkusiak
Copy link
Contributor

@3mkusiak 3mkusiak commented Sep 8, 2025

The logic for checking if test should fail by default is reversed, thus all passes or errors are treated as unknowns. Enforce checking the keyword "UNKNOWN" for the tests.

Fixes:
Dasharo/dasharo-issues#1304

@3mkusiak 3mkusiak requested a review from m-iwanicki September 8, 2025 08:08
Copy link
Contributor

@m-iwanicki m-iwanicki left a comment

Choose a reason for hiding this comment

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

One small change, apart from that it looks good:

[OK]            PCI configuration space and topology
[OK]            USB devices and topology
[OK]            Super I/O configuration
[OK]            EC configuration
[ERROR]         MSRs
[OK]            SMBIOS tables
[OK]            BIOS information
[OK]            CMOS NVRAM
[UNKNOWN]       Intel configuration registers
[OK]            GPIO configuration C header files
[OK]            kernel dmesg
[OK]            ACPI tables
[UNKNOWN]       Audio devices configuration
[OK]            CPU info
[OK]            I/O ports
[OK]            Input bus types
[ERROR]         Firmware image
[OK]            I2C bus
[UNKNOWN]       ACPI tables
[OK]            Touchpad information
[ERROR]         DIMMs information
[UNKNOWN]       CBMEM table information
[UNKNOWN]       CBMEM console
[ERROR]         TPM information
[UNKNOWN]       AMT information
[OK]            ME information
[UNKNOWN]       Graphics VBT

There is still problem that some items will always be unknown as we are always passing UNKNOWN string to this function e.g.

  • Audio devices configuration
  • ACPI tables - the second one (I think it's duplicate)
  • Graphics VBT

I think to close the issue 100% you would have to fix that too

The logic for checking if test should fail by default is reversed, thus
all passes or errors are treated as unknowns. Enforce checking the
keyword "UNKNOWN" for the tests.

Fixes:
Dasharo/dasharo-issues#1304

Signed-off-by: Mateusz Kusiak <mateusz.kusiak@3mdeb.com>
@3mkusiak 3mkusiak changed the title reports: enforce UNKNOWN keyword WIP: reports: enforce UNKNOWN keyword Sep 11, 2025
@m-iwanicki m-iwanicki marked this pull request as draft September 11, 2025 09:37
@DaniilKl
Copy link
Contributor

DaniilKl commented Sep 11, 2025

One small change, apart from that it looks good:

[OK]            PCI configuration space and topology
[OK]            USB devices and topology
[OK]            Super I/O configuration
[OK]            EC configuration
[ERROR]         MSRs
[OK]            SMBIOS tables
[OK]            BIOS information
[OK]            CMOS NVRAM
[UNKNOWN]       Intel configuration registers
[OK]            GPIO configuration C header files
[OK]            kernel dmesg
[OK]            ACPI tables
[UNKNOWN]       Audio devices configuration
[OK]            CPU info
[OK]            I/O ports
[OK]            Input bus types
[ERROR]         Firmware image
[OK]            I2C bus
[UNKNOWN]       ACPI tables
[OK]            Touchpad information
[ERROR]         DIMMs information
[UNKNOWN]       CBMEM table information
[UNKNOWN]       CBMEM console
[ERROR]         TPM information
[UNKNOWN]       AMT information
[OK]            ME information
[UNKNOWN]       Graphics VBT

There is still problem that some items will always be unknown as we are always passing UNKNOWN string to this function e.g.

* `Audio devices configuration`

* `ACPI tables` - the second one (I think it's duplicate)

* `Graphics VBT`

I think to close the issue 100% you would have to fix that too

@3mkusiak, have you checked, why we are not providing the error log file as a second argument to the update_result function in these 3 cases? Maybe the reason is, that these 3 checks do not produce output, basing on which we can conclude the status. If that is the case - then it is not an issue but a feature and you do not need to fix it. Please, provide a confirmation for my assumption.

If my assumption is correct - provide comments near the update_result calls explaining the situation, so we wont waste time guessing in future.

@3mkusiak
Copy link
Contributor Author

Maybe the reason is, that these 3 checks do not produce output, basing on which we can conclude the status. If that is the case - then it is not an issue but a feature and you do not need to fix it. Please, provide a confirmation for my assumption.

Dunno how about the rest, but the ACPI tables one silently fails, so I wouldn't call it a feature. Either way, Imo we should let it fail, not enforce unknowns, this is just a workaround for lazy coding. Dunno, why it's implemented the way it is as there are no descriptions in commits.

@DaniilKl
Copy link
Contributor

DaniilKl commented Sep 11, 2025

Dunno, why it's implemented the way it is as there are no descriptions in commits.

Does not matter how it is implemented, if it works as we expect it should. Does the missing log files an expected behaviour in these 3 cases?

@m-iwanicki
Copy link
Contributor

m-iwanicki commented Sep 11, 2025

@DaniilKl it's likely because all 3 dump more than 1 log and update_result function expects only one.
Also those ACPI tables are not duplicates and do something different, so I think they should have different name

@DaniilKl
Copy link
Contributor

DaniilKl commented Sep 11, 2025

it's likely because all 3 dump more than 1 log and update_result function expects only one.

Why can't we use any of the log files to check the status? Or use all three log files, but name the statuses for every log file differently?

@3mkusiak
Copy link
Contributor Author

Does the missing log files an expected behaviour in these 3 cases?

what

I assume you ask if it's expected these commands do not produce logs. They don't, not the "classic" form we expect at least, but this is on our side to collect this logs. The tasks might silently fail and will never fix them. I might as well, remove printing updates for these actions and just perform them in silence.

The current progress bar implementation disallows for easy moving tasks
around and adding or removing them. Implement dynamic progress bar,
so the tasks can be easily altered.

Signed-off-by: Mateusz Kusiak <mateusz.kusiak@3mdeb.com>
@3mkusiak 3mkusiak changed the title WIP: reports: enforce UNKNOWN keyword reports: enforce UNKNOWN keyword Sep 11, 2025
@3mkusiak 3mkusiak marked this pull request as ready for review September 11, 2025 12:56
@DaniilKl
Copy link
Contributor

@3mkusiak, open issues for these 3 cases describing the situation in details and link the issue in the source code as a comment. We will get to them later.

3 tasks in hcl report logic are enforced to finish with UNKNOWN status,
due to the fact they not produce single STDERR and STDOUT logs. The
issues have been submitted. Link those issues in the source code.

Signed-off-by: Mateusz Kusiak <mateusz.kusiak@3mdeb.com>
@3mkusiak
Copy link
Contributor Author

Repro:
rpr.txt

Copy link
Contributor

@m-iwanicki m-iwanicki left a comment

Choose a reason for hiding this comment

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

Works, tested on QEMU without emulation:

[OK]            PCI configuration space and topology
[UNKNOWN]       USB devices and topology
[OK]            Super I/O configuration
[UNKNOWN]       EC configuration
[ERROR]         MSRs
[OK]            SMBIOS tables
[OK]            BIOS information
[OK]            CMOS NVRAM
[UNKNOWN]       Intel configuration registers
[OK]            GPIO configuration C header files
[OK]            kernel dmesg
[OK]            ACPI tables
[UNKNOWN]       Audio devices configuration
[OK]            CPU info
[OK]            I/O ports
[OK]            Input bus types
[OK]            I2C bus
[UNKNOWN]       ACPI tables
[OK]            Touchpad information
[ERROR]         DIMMs information
[ERROR]         CBMEM table information
[ERROR]         CBMEM console
[ERROR]         TPM information
[ERROR]         AMT information
[OK]            ME information
[UNKNOWN]       Graphics VBT

@m-iwanicki m-iwanicki merged commit f504940 into main Sep 12, 2025
1 check passed
@m-iwanicki m-iwanicki deleted the fix_hcl_rep branch September 12, 2025 07:46
3mkusiak added a commit to Dasharo/meta-dts that referenced this pull request Sep 12, 2025
This change bumps dts version to address following issues:
* Fix dpp update issue, in which update defaulted to heads:
  Dasharo/dts-scripts#106
* Fix hcl report issue, in which all checks defaulted to UNKNOWN and
  implement dynamic progress bar, which shows current task number:
  Dasharo/dts-scripts#107

Signed-off-by: Mateusz Kusiak <mateusz.kusiak@3mdeb.com>
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