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

[BUG] #5027 provides changes for logger blocking compilation with MSYS2 #6115

Closed
pjdobrowolski opened this issue Aug 10, 2022 · 19 comments
Closed
Assignees
Labels
bug Something isn't working as expected

Comments

@pjdobrowolski
Copy link
Contributor

Changes made in #5027 require inotify which is available Linux Only. What blocks compilation.

Possible solution:

  • Switching off logger from default west compilation.
  • Revert commit and use different solution
  • Add ifdefs to turnoff that feature on Windows
@pjdobrowolski pjdobrowolski added the bug Something isn't working as expected label Aug 10, 2022
@lgirdwood
Copy link
Member

@pjdobrowolski can you ifdef atm for MSYS2 ? Sof logger is not the long term tool as Zephyr tooling will be used for log and trace. If you dont need to run it on MSYS2 then faster to remove from compilation if building on MSYS2

@marc-hb
Copy link
Collaborator

marc-hb commented Aug 15, 2022

Compiling sof-logger on Windows was always a long shot. Most of what the sof-logger code does is reading and parsing /sys/kernel/debug/sof/ which is never going to work on Windows. It can also decode a recorded trace off-line but who's going to capture on Linux and decode on Windows?

I recommend disabling sof-logger compilation in xtensa-build-zephyr.py when run on Windows. Should be the fastest fix and very easy to revert in the very unlikely case someone wants to restore Windows compatibility.

@lgirdwood
Copy link
Member

but who's going to capture on Linux and decode on Windows?

Intel has engineers who will want to do this, we need to work this out.

@kfrydryx kfrydryx self-assigned this Aug 16, 2022
@aborisovich
Copy link
Contributor

aborisovich commented Aug 16, 2022

@marc-hb can you please fix the issue with logger by ifdefing this missing on MSYS2 header and feature?
Reading debug-fs is not applicable to Windows anyway. I think it should be all 'ifdefed' somehow as 'linux only'.

@marc-hb
Copy link
Collaborator

marc-hb commented Aug 17, 2022

Intel has engineers who will want to do this, we need to work this out.

OK, I'm quite surprised by this considering the earlier comments below. Change of mind?

Sof logger is not the long term tool as Zephyr tooling will be used for log and trace. If you dont need to run it on MSYS2 then faster to remove from compilation if building on MSYS2

@marc-hb can you please fix the issue with logger by ifdefing this missing on MSYS2 header and feature?

I'd like to but I'm on sort-of-vacation right now and even when at work I don't have any Windows environment to test this on. Sorry to insist but I think disabling sof-logger compilation in the script is really the fastest, smallest and easy to revert short-term workaround until someone can provide the right #ifdef (any volunteer?) Again I don't think anyone has ever used this (soon deprecated) tool on Windows yet.

@abonislawski
Copy link
Member

Again I don't think anyone has ever used this (soon deprecated) tool on Windows yet.

We are using it from the very beginning on windows with python tests just to parse input file

@marc-hb
Copy link
Collaborator

marc-hb commented Aug 18, 2022

We are using it from the very beginning on windows with python tests just to parse input file

I didn't know sorry. This should really be in some CI then.

Not sure what you mean by "very beginning" but I remember @aborisovich struggling to compile it with MSYS2 6 months ago, see

What "input file" are you referring to? .ldc dictionary?

@aborisovich
Copy link
Contributor

aborisovich commented Aug 18, 2022

We are using it from the very beginning on windows with python tests just to parse input file

I didn't know sorry. This should really be in some CI then.

Not sure what you mean by "very beginning" but I remember @aborisovich struggling to compile it with MSYS2 6 months ago, see

What "input file" are you referring to? .ldc dictionary?

Yes, option sof-logger -d worked as far as I remember
image

@abonislawski
Copy link
Member

Not sure what you mean by "very beginning" but I remember @aborisovich struggling to compile it with MSYS2 6 months ago, see

sof-logger is much older than 5 months, in the past it was working fine with cygwin build, my first tries are from 2018 :) and it was used under windows to parse tgl, cnl and older platforms logs. On MTL we are using sof-logger too (but only as temporary debug solution).

What "input file" are you referring to? .ldc dictionary?

No, input file contains logs, substitute of debugfs trace file in linux. Python tests with diag driver can save dma output as .bin file and then we can parse it using sof-logger and .ldc dictionary.
You dont need to work with linux debugfs, you can also use sof-logger as parser when all files provided, check sof-logger help:

-i infile -o outfile -l *.ldc_file
Dump infile contents to outfile

@marc-hb
Copy link
Collaborator

marc-hb commented Aug 18, 2022

in the past it was working fine with cygwin build, my first tries are from 2018 :)

OK, that must be why #5299 was called "resurrect" then :)

Thanks for the additional details, sof-logger should really be compiled in some Windows CI then, otherwise it will keep bitrotting and breaking regularly.

I will try to #ifdef linux the inotify bits in the next few days[*], it should be easy enough but due to the Windows CI gap I will need someone else to actually test it on Windows (until it breaks again).

EDIT: I've been sick, sorry for the delay.

@marc-hb
Copy link
Collaborator

marc-hb commented Aug 18, 2022

will try to #ifdef linux the inotify bits in the next few days, it should be easy enough

Actually: what #ifdef "magic" should be used here? In other words, how does one make the difference between Linux versus Cygwin versus MSYS2? Considering the last two try hard to impersonate Linux on Windows...

This won't be blocking me but I'll need to know eventually.

@abonislawski
Copy link
Member

abonislawski commented Aug 19, 2022

Best if can be solved inside but if not: maybe ifdef for this feature which depends on additional build parameter (in xtensa-build-zephyr.py there is check linux vs windows)?
but this is not really an urgency, on our fpga platforms we have old builds from 2021 with cygwin1.dll :) So Im not sure if newest build with msys2 was ever used (maybe never will if this tool will be obsolete soon), there is rarely need to update if it just works.

Thats why I have no idea why it is building every time with each fw build, we should disable this anyway.

@kfrydryx
Copy link
Contributor

Actually: what #ifdef "magic" should be used here?

@marc-hb MSYS porting guide might be helpful, fe. there is a list of platform checks https://www.msys2.org/wiki/Porting/

@lgirdwood
Copy link
Member

@marc-hb @aborisovich @kfrydryx @abonislawski Is there a simple path to run basic WSL2 GH actions to prevent Windows failures ? I found https://github.com/features/actions which seems to indicate we can have a Windows container. However, I'm not a Windows user and no idea if this Windows container configuration would be suitable.

@aborisovich
Copy link
Contributor

aborisovich commented Aug 19, 2022

Yes this can be done but somebody should start from Proof of Concept task and build locally project on Windows using new Zephyr SDK for Windows. I think nobody tried to do this. If this works we can easily add new processes for windows-latest container(I think it is Windows Server 2019) install there MSYS2. Then we can add MSYS and Zephyr sdk to cache https://github.com/marketplace/actions/cache so they are not reinstalled on every CI run. It looks like chocolatey package manager has MSYS2 package so it should be easy to install. Installing Zephyr SDK will be a bit harder probably but doable.

@marc-hb
Copy link
Collaborator

marc-hb commented Aug 25, 2022

Fix submitted in #6191. Someone should try to compile #6191 with MSYS. I went for the HAS_INOTIFY route which easily let me lie and pretend I didn't have inotify for local testing purposes so I'm pretty confident it should work on MSYS too.

@aborisovich
Copy link
Contributor

I'll test it in a moment

@pjdobrowolski
Copy link
Contributor Author

pjdobrowolski commented Aug 29, 2022

Checked and tested. Fix works.

@aborisovich
Copy link
Contributor

Yep, it works, sorry for late reply... Thanks @pjdobrowolski .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected
Projects
None yet
Development

No branches or pull requests

6 participants