-
Notifications
You must be signed in to change notification settings - Fork 293
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
Update CI #619
Update CI #619
Conversation
The CI does not need to strictly adhere to the coding rules. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general looks good, some nits you should fix.
And then decide to tackle the dir structure or not
-DCMAKE_INSTALL_PREFIX=$PROJECT_ROOT/target \ | ||
-DCMAKE_C_FLAGS="-Werror -Wall -Wextra -Wshadow -Wunused-but-set-variable" || exit 1 | ||
make -C build VERBOSE=1 install || exit 1 | ||
exit 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: exit 0 here has mixed indents. It was probibly that way originally but since you touched the line, please fit it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
make VERBOSE=1 all || exit 1 | ||
exit 0 | ||
export PROJECT_ROOT=$PWD | ||
#Build libmetal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might as well make these comments echo statments so we can see better what is going on. I use something like
echo "****** build libmetal"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cmake . -Bbuild -DCMAKE_INSTALL_PREFIX=$PROJECT_ROOT/target \ | ||
-DCMAKE_C_FLAGS="-Werror -Wall -Wextra -Wshadow -Wunused-but-set-variable" || exit 1 | ||
make -C build VERBOSE=1 install || exit 1 | ||
#Build open_amp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
cd $PROJECT_ROOT/libmetal && | ||
cmake . -Bbuild -DCMAKE_INSTALL_PREFIX=$PROJECT_ROOT/target \ | ||
-DCMAKE_C_FLAGS="-Werror -Wall -Wextra -Wshadow -Wunused-but-set-variable" || exit 1 | ||
make -C build VERBOSE=1 install || exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want verbose?? It gives more info when things go wrong but hides warnings when things go right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
@@ -23,12 +23,17 @@ jobs: | |||
runs-on: ubuntu-latest | |||
steps: | |||
- name: Checkout open-amp | |||
uses: actions/checkout@v3 | |||
uses: actions/checkout@v4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"path: open-amp" HERE ???
with adjustments in script?
I think libmetal and openamp-system-reference are sub directories of open-amp. That works but is a little strange. Is this easy to fix as described above?? If not we will deal with it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take this comment back. Every build would need to be updated not just the linux build.
We should do this in another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"path: open-amp" HERE ??? with adjustments in script?
I think libmetal and openamp-system-reference are sub directories of open-amp. That works but is a little strange. Is this easy to fix as described above?? If not we will deal with it later.
Not straightforward as expected but I added a commit to address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take this comment back. Every build would need to be updated not just the linux build. We should do this in another PR
extra commit added in the PR
Use last version of checkout action. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
To have a coherent directory hierarchy, move the open-amp git from root to the "./open-amp" directory. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
Build system reference applications instead of open-amp deprecated applications. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed this and tested it as well.
I think it is ready.
Update CI to build the https://github.com/OpenAMP/openamp-system-reference/tree/main/examples/legacy_apps applications instead of the https://github.com/OpenAMP/open-amp/tree/main/apps applications, which will be deprecated in the next release.