Skip to content

Fix syntax errors in some pre-removal and post-installation scripts #8046

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix syntax errors in some pre-removal and post-installation scripts #8046

wants to merge 2 commits into from

Conversation

@keryell
Copy link
Contributor

@keryell keryell commented Apr 3, 2024

This was introduced with commit
2b87027 from
#8008

@keryell keryell requested a review from sonals April 3, 2024 00:10
@keryell keryell requested review from rozumx and stsoe as code owners April 3, 2024 00:10
@keryell
Copy link
Contributor Author

keryell commented Apr 3, 2024

Probably the CI does not look at install script execution?
Or some systems have bash as sh?

@keryell keryell force-pushed the no-bashism-postinst branch from 2ff841a to ad7b6ee Compare April 3, 2024 00:12
@keryell keryell mentioned this pull request Apr 3, 2024
Closed
@keryell keryell force-pushed the no-bashism-postinst branch from 5e3b272 to 975e5c3 Compare April 3, 2024 01:23
@keryell
Copy link
Contributor Author

keryell commented Apr 3, 2024

OK, the problem seems worse.
It looks like the CI is probably broken since #7835
@zhangchiming It is unclear how it is possible.

@keryell keryell requested a review from zhangchiming April 3, 2024 01:24
@keryell keryell changed the title Remove bash-ism from post-installation script which breaks install Fix syntax errors in some pre-removal and post-installation scripts Apr 3, 2024
zhangchiming
zhangchiming approved these changes Apr 3, 2024
View reviewed changes
Copy link
Collaborator

@zhangchiming zhangchiming left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

@keryell
Copy link
Contributor Author

keryell commented Apr 3, 2024

There is still a failing XRT test
https://github.com/Xilinx/XRT/actions/runs/8531822644/job/23372394605?pr=8046
pcie-hw-tests (xilinx_u250_gen3x16_xdma_4_1_202210_1) with
http://fisweb:8080/proj/fis/scripts/buildNumber:571_1_boardfarm_xilinx_u250_gen3x16_xdma_4_1_202210_1.log

2024-04-02 18:47:21,458 - root - INFO - b'2024.04.02.18.47.21 INFO: ERROR: Post-check FAILED : [Install_XRT] : XRT did not compile drivers on xcosdae368\n'
1021	2024-04-02 18:47:21,463 - root - INFO - b'2024.04.02.18.47.21 INFO: ERROR: ERROR: Failed to run step Install_XRT in Main flow pipeline_2rp_no_xrm: XRT did not compile drivers on xcosdae368\n'

I do not have access to the compilation logs because I cannot log into the machine (the ssh hangs forever).
I am also concerned by this machine using Linux 4.18 from 2018. ☹️

@keryell keryell force-pushed the no-bashism-postinst branch from 975e5c3 to ce9f767 Compare April 8, 2024 18:15
@keryell
Copy link
Contributor Author

keryell commented Apr 8, 2024 •
edited
Loading

Rebased on master to see whether it fixes the CI.

@keryell
Copy link
Contributor Author

keryell commented Apr 12, 2024

@sonals @stsoe Do you have a plan to make progress on this?

@keryell keryell mentioned this pull request Apr 12, 2024
Merged
@keryell keryell force-pushed the no-bashism-postinst branch from ce9f767 to a913afd Compare April 16, 2024 15:58
@keryell
Copy link
Contributor Author

keryell commented Apr 16, 2024

Rebasing on master after 2 weeks to see whether it fixes the CI.

@keryell keryell force-pushed the no-bashism-postinst branch from a913afd to ac1f148 Compare April 23, 2024 18:34
@keryell keryell mentioned this pull request Sep 13, 2024
Merged
@stsoe
Copy link
Collaborator

stsoe commented Sep 13, 2024

@keryell Can you rebase and force push? Seems like there is no way to restart CI for this old PR.

@keryell
Remove bash-ism from post-installation script which breaks install
d02a61c 
This was introduced with commit
2b87027 from
Xilinx#8008

Signed-off-by: Ronan Keryell <[email protected]>
@keryell
Fix CMake syntax bug which breaks package pre-removal
a39f7ec 
Fix bugs introduced in commit 4747733
by Xilinx#7835

Signed-off-by: Ronan Keryell <[email protected]>
@keryell keryell force-pushed the no-bashism-postinst branch from ac1f148 to a39f7ec Compare September 19, 2024 00:56
@keryell
Copy link
Contributor Author

keryell commented Sep 19, 2024

The CI looks broken: it accepted some bugs in the past and trying to remove them breaks the CI??? 🤔

@stsoe
Copy link
Collaborator

stsoe commented Feb 20, 2025

Close and reopen to trigger CI

@stsoe stsoe closed this Feb 20, 2025
@stsoe stsoe reopened this Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhangchiming zhangchiming zhangchiming approved these changes

@sonals sonals Awaiting requested review from sonals

@rozumx rozumx Awaiting requested review from rozumx rozumx is a code owner

@stsoe stsoe Awaiting requested review from stsoe stsoe is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@keryell @stsoe @zhangchiming