Skip to content

Conversation

@marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Apr 26, 2023

Remove one intermediate and unnecessary .ri file. Simplify the code and the build directory. Example below with sof-mtl.ri for better
readability but this applies the same to sof-imx8.ri and every other
sof-$platf.ri file.

There were 3 .ri copies before this commit:

                build-mtl/zephyr/zephyr.ri
copied to  ->   build-mtl/zephyr/sof-mtl.ri
copied to  ->   build-sof-staging/___/sof-mtl.ri

Only 2 left after dropping the second and pointless copy:

                build-mtl/zephyr/zephyr.ri
copied to  ->   build-sof-staging/___/sof-mtl.ri

Fewer copies means less wondering about what is what, two identical files in the same directory is at best pointless and at worst misleading.

The build-mtl/ directory belongs to the zephyr build system exclusively, this wrapper script has no business interfering with build-mtl. `build-mtl/zephyr/sof-mtl.ri can even be dangerous because it does not get cleaned. Demonstration:

 # Compile successfully
 $ ./scripts/xtensa-build-zephyr.py mtl

 # Write some code and make a mistake.
 # Fail to compile.
 $ echo BROKEN >> ../zephyr/kernel/sched.c
 $ ./scripts/xtensa-build-zephyr.py mtl

 # Obsolete .ri files are still there
 $ find ../build-mtl/ -name *.ri
  ../build-mtl/zephyr/zephyr.ri
  ../build-mtl/zephyr/sof-mtl.ri

 $ ninja -C ../build-mtl clean

 # Obsolete sof-mtl.ri is still there!
 $ find ../build-mtl/ -name *.ri
  ../build-mtl/zephyr/sof-mtl.ri

@marc-hb
Copy link
Collaborator Author

marc-hb commented Apr 26, 2023

Zephyr main branch failures in https://github.com/thesofproject/sof/actions/runs/4814369445/jobs/8571944751 will be fixed by #7520. Manifest Zephyr version is green.

https://sof-ci.01.org/sofpr/PR7522/build6524/devicetest and https://sof-ci.01.org/sofpr/PR7522/build6525/devicetest have only a few unrelated suspend/resume timeouts and one minor systemctl issue. Everything else is OK.

@marc-hb marc-hb marked this pull request as ready for review April 27, 2023 00:00
@marc-hb marc-hb requested a review from aborisovich as a code owner April 27, 2023 00:00
Copy link
Collaborator

@paulstelian97 paulstelian97 left a comment

Choose a reason for hiding this comment

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

Commit message is oddly specific about mtl, yet it affects all platforms. Please make the message clearer in this sense.

I approve of the change itself.

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

This will break tons of my old DIY scripts, but my bad not to have upgraded them.

Otherwise good, but agree that the commit should be clearer this applies to all targets.

Remove one intermediate and unnecessary .ri file. Simplify the code and
the build directory. Example below with `sof-mtl.ri` for better
readability but this applies the same to `sof-imx8.ri` and every other
`sof-$platf.ri` file.

There were 3 .ri copies before this commit:

```
                build-mtl/zephyr/zephyr.ri
copied to  ->   build-mtl/zephyr/sof-mtl.ri
copied to  ->   build-sof-staging/___/sof-mtl.ri
```

Only 2 left after dropping the second and pointless copy:

```
                build-mtl/zephyr/zephyr.ri
copied to  ->   build-sof-staging/___/sof-mtl.ri
```

Fewer copies means less wondering about what is what, two identical
files in the same directory is at best pointless and at worst misleading.

The `build-mtl/` directory belongs to the zephyr build system
exclusively, this wrapper script has no business interfering with
`build-mtl`. `build-mtl/zephyr/sof-mtl.ri can even be dangerous because
it does not get cleaned. Demonstration:

```
 # Compile successfully
 $ ./scripts/xtensa-build-zephyr.py mtl

 # Write some code and make a mistake.
 # Fail to compile.
 $ echo BROKEN >> ../zephyr/kernel/sched.c
 $ ./scripts/xtensa-build-zephyr.py mtl

 # Obsolete .ri files are still there
 $ find ../build-mtl/ -name *.ri
  ../build-mtl/zephyr/zephyr.ri
  ../build-mtl/zephyr/sof-mtl.ri

 $ ninja -C ../build-mtl clean

 # Obsolete sof-mtl.ri is still there!
 $ find ../build-mtl/ -name *.ri
  ../build-mtl/zephyr/sof-mtl.ri

```

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb marc-hb changed the title xtensa-build-zephyr.py: drop intermediate build-mtl/sof-mtl.ri file xtensa-build-zephyr.py: drop intermediate build-$platf/sof-$platf.ri Apr 28, 2023
@marc-hb
Copy link
Collaborator Author

marc-hb commented Apr 28, 2023

Commit message adjusted, thanks for the reviews!

This will break tons of my old DIY scripts, but my bad not to have upgraded them.

Sorry about that but build-sof-staging/ has been longing for you for at least a year now!

May I also suggest taking the opportunity to reduce the "tons" of scripts to a much smaller number?

@marc-hb
Copy link
Collaborator Author

marc-hb commented May 2, 2023

One model was missing in https://sof-ci.01.org/sofpr/PR7522/build6712/devicetest/index.html, others ran fine.

One suspend/resume in https://sof-ci.01.org/sofpr/PR7522/build6713/devicetest/index.html, unrelated too.

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Thanks for the commit update!

@kv2019i kv2019i requested a review from paulstelian97 May 4, 2023 13:58
@kv2019i
Copy link
Collaborator

kv2019i commented May 4, 2023

@paulstelian97 @aborisovich ... ok?

Copy link
Contributor

@aborisovich aborisovich left a comment

Choose a reason for hiding this comment

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

Yeah makes sense. No need for so many copies.

@aborisovich
Copy link
Contributor

@marc-hb please double check CI test results. I hope tests are not failing because could not be started, because looking for sof-mtl.ri in build-mtl directory...

@marc-hb
Copy link
Collaborator Author

marc-hb commented May 4, 2023

One model was missing in https://sof-ci.01.org/sofpr/PR7522/build6712/devicetest/index.html, others ran fine.

As far as I know we build a single sof-mtl.ri and other MTL models ran fine but OK, let's try again to be on the safe side.

@marc-hb
Copy link
Collaborator Author

marc-hb commented May 4, 2023

SOFCI TEST

@marc-hb
Copy link
Collaborator Author

marc-hb commented May 4, 2023

Both https://sof-ci.01.org/sofpr/PR7522/build7014/devicetest and https://sof-ci.01.org/sofpr/PR7522/build7013/devicetest are fine with tests running on all MTL models. Just some usual suspend/resume failures.

@kv2019i kv2019i merged commit a4ece86 into thesofproject:main May 5, 2023
@marc-hb marc-hb deleted the drop-sof-ri-copy branch May 9, 2023 00:49
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.

5 participants