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

Fix broken CI #586

Merged
merged 1 commit into from
May 7, 2024
Merged

Fix broken CI #586

merged 1 commit into from
May 7, 2024

Conversation

arnopo
Copy link
Collaborator

@arnopo arnopo commented May 3, 2024

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
Copy link
Collaborator

wmamills commented May 4, 2024

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
Copy link
Collaborator

wmamills commented May 4, 2024

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
Copy link
Collaborator

wmamills commented May 5, 2024

@arnopo
If you wish to stay with Ubuntu 24.04, I suggest this version of the change.
wmamills@a4a2d98

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
Copy link
Contributor

glneo commented May 5, 2024

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
Copy link
Collaborator Author

arnopo commented May 6, 2024

@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
Copy link
Collaborator Author

arnopo commented May 6, 2024

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
Copy link
Collaborator

wmamills commented May 6, 2024

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.

Copy link
Collaborator

@wmamills wmamills left a comment

Choose a reason for hiding this comment

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

If we are going to use the venv then it would be best to lose the ~/.local/bin stuff
Delete out of commit header also (can't write a review comment for the header)

I will leave this as a comment as I will leave it up to you to do this or take as is.


ENV LANG C.UTF-8
ENV LC_ALL C.UTF-8

# If this dir exists, then global bashrc scripts will take care of adding it to the path
Copy link
Collaborator

Choose a reason for hiding this comment

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

This section is no longer needed

ln -s /usr/share/zoneinfo/Etc/UTC /etc/localtime || exit 1
ln -fs /usr/share/zoneinfo/Etc/UTC /etc/localtime || exit 1

# use ~/.local/bin even for non-interactive shells
Copy link
Collaborator

Choose a reason for hiding this comment

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

This section is no longer needed

@arnopo arnopo force-pushed the fix_CI branch 2 times, most recently from d74e169 to 4cab6af Compare May 6, 2024 14:11
@arnopo
Copy link
Collaborator Author

arnopo commented May 6, 2024

I replaced pip by pip3

@arnopo arnopo requested a review from wmamills May 6, 2024 14:12
@glneo
Copy link
Contributor

glneo commented May 6, 2024

I replaced pip by pip3

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

Copy link
Collaborator

@wmamills wmamills left a comment

Choose a reason for hiding this comment

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

This looks good as long as PR# 587 is accepted first

Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

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

Looks good to go.

Update CI to work with 24.04.

* Lock to 24.04 so we control when to change
* Update locale and timezone to work for 24.04 or 22.04
  * 24.04 creates these files if not already there but 22.04 does not
  * Keep existing work around for 22.04 but make it work if already there
* use venv
  * Breate a virtual environment isolated from the packages in the base
    environment
  * Inspired from zephyr build environment
* Use pip3 everywhere removing global and --user installation
* Add section to README about desktop testing
* Also fix usage formatting in README

Co-developed-by: Bill Mills <bill.mills@linaro.org>
Signed-off-by: Bill Mills <bill.mills@linaro.org>
Co-developed-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
@arnopo arnopo merged commit e5cc3a3 into OpenAMP:main May 7, 2024
2 of 3 checks passed
@arnopo arnopo added this to the Release V2024.04 milestone May 7, 2024
@arnopo arnopo deleted the fix_CI branch May 7, 2024 07:22
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