OpenAMP / open-amp

The main OpenAMP library implementing RPMSG, Virtio, and Remoteproc for RTOS etc
https://www.openampproject.org/
Other
678 stars 278 forks source link

Fix broken CI #586

Closed arnopo closed 1 month ago

arnopo commented 2 months ago

The "open-amp lib Continuous Integration / check builds on different platforms" check has failed due to a Docker environment update.

Issues were found related to timezone settings and pip3. Upon fixing these, a build error was reported by GCC (likely a new version) in rpmsg_rpc_client.c.

This PR addresses both issues to restore a working CI environment.

wmamills commented 2 months ago

The 2nd commit looks fine but it is a library code fix so I don't think it should come in the same PR as "Fix CI". (This is visible in the release notes if you use the github release note creation help)

The big problem with the CI files is it uses ubuntu:latest in the Dockerfile and thus we are at the whim of Ubuntu (LTS) releases. I did verify that we are getting 24.04 right now.

I suggest you change ubuntu:latest to ubuntu:22.04 for now. Or if you prefer we can do 24.04 but we should be in control. If you use 22.04, the other changes should not be needed, I assume. (Update: verified that this single change is all that is needed to work on 22.04)

I never liked the env handling in this setup as it is split between the Dockerfile and the entrypoint.sh. A clean 24.04 container has nothing in /etc/timezone and /etc/localtime so I will need to figure what new thing is happening and why.

I would change: ln -s /usr/... to ln -fs /usr/... so it would work the same with ubuntu 22.04 and 24.04.

I don't think --break-system-packages should be needed if you use --user. I would suggest we use --user for them all instead of mixing them as they now are. (Update: FALSE, still needed)

I will try these out and let you know.

wmamills commented 2 months ago

Unfortunately even --user requires the --break-system-packages at least at the moment. Some people think (as I do ) that it should not be needed by --user. The argument that --user can break distro packages is very week to me as they are only in the path for interactive shells. You have to explicitly opt into them otherwise. Anything in /usr/local is far worse than something in ~/.local/bin.

wmamills commented 2 months ago

@arnopo If you wish to stay with Ubuntu 24.04, I suggest this version of the change. https://github.com/wmamills/open-amp/commit/a4a2d98df16a5cf9ff4baf8815728ee65cf4299b

I have tested this with github actions and with desktop testing.

If you agree I will do the same for libmetal.

If you agree, do you want a new PR or do you want me to force push to this one after you separate the lib code fix?

glneo commented 2 months ago

System packages can still be "broken" by user installs from the user perspective. Even though the overridden packages only apply in interactive shells, you still don't want to break your user shell with mismatched packages either. If we need specific packages, the recommendation is venv. We should switch CI to that at some point.

For now, cleanest approach is just lock to 22.04, so +1 for that.

arnopo commented 1 month ago

@wmamills Thanks for the update ! I take your patch and update it to use venv as proposed by @glneo. I also remove --user and use pip instead of pip3 , inspired from https://docs.zephyrproject.org/latest/develop/getting_started/index.html#get-zephyr-and-install-python-dependencies

arnopo commented 1 month ago

The 2nd commit looks fine but it is a library code fix so I don't think it should come in the same PR as "Fix CI". (This is visible in the release notes if you use the github release note creation help)

I keep it in this PR to have the CI pass. I will remove it before merging. I created #587

wmamills commented 1 month ago

If you create a venv then all the stuff to handle ~/.local/bin is really not needed.

Yes, once your are in an activated venv it knows which python version you want and you can use pip instead of pip3 but I am not sure everyone will understand why that is safe now.

Which approach we use here in this contained environment is not really important but the command lines look cleaner now. I do agree using a venv whenever you have a -r file is a good idea. I strongly disagree that --user is always bad.

arnopo commented 1 month ago

I replaced pip by pip3

glneo commented 1 month ago

I replaced pip by pip3

python3 -m pip would be even more correct, but now we are really bikeshedding :smile: