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

amdgpu: Set PLUGINDIR to /usr/lib/criu #1876

Merged
merged 1 commit into from
May 17, 2022

Conversation

rst0git
Copy link
Member

@rst0git rst0git commented May 8, 2022

This patch fixes the following error which appears when building criu packages for Ubuntu/Debian.

mkdir: cannot create directory '/var/lib/criu': Permission denied

@rst0git
Copy link
Member Author

rst0git commented May 8, 2022

cc: @rajbhar

@codecov-commenter
Copy link

codecov-commenter commented May 8, 2022

Codecov Report

Merging #1876 (795246e) into criu-dev (37ea8c5) will increase coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           criu-dev    #1876   +/-   ##
=========================================
  Coverage     68.98%   68.98%           
=========================================
  Files           128      129    +1     
  Lines         33346    33349    +3     
=========================================
+ Hits          23003    23006    +3     
  Misses        10343    10343           
Impacted Files Coverage Δ
criu/uffd.c 79.36% <0.00%> (-0.48%) ⬇️
criu/include/vma.h 100.00% <0.00%> (ø)
criu/include/criu-log.h 100.00% <0.00%> (ø)
criu/cr-restore.c 67.25% <0.00%> (+0.18%) ⬆️
criu/plugin.c 24.21% <0.00%> (+0.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37ea8c5...795246e. Read the comment docs.

@jpalus
Copy link

jpalus commented May 8, 2022

Would be good if this install target respected DESTDIR as well.

@adrianreber
Copy link
Member

I am not really happy with the plugin location in /var. I think it should be somewhere in /usr.

@jpalus
Copy link

jpalus commented May 8, 2022

To add even more concerns regarding amdgpu plugin:

  • PLUGINDIR from Makefile should be used as #define CR_PLUGIN_DEFAULT value via build flag -DCR_PLUGIN_DEFAULT=\"$(PLUGINDIR)\"
  • not sure where drm.h header comes from in #include <drm/drm.h>
  • pkgconfig --cflags libdrm flags are not used

@rst0git rst0git changed the title amdgpu: Create PLUGINDIR only when necessary amdgpu: Set PLUGINDIR to /usr/lib/criu May 8, 2022
@rst0git
Copy link
Member Author

rst0git commented May 8, 2022

@jpalus and @adrianreber Thank you for your comments. I've updated the pull request.

@adrianreber
Copy link
Member

This makes more sense to me than the previous location. 👍

Please rebase to pick up your CI linter run fix.

@rst0git rst0git force-pushed the plugindir branch 3 times, most recently from e58efac to 6f92d01 Compare May 10, 2022 20:54
@avagin avagin requested a review from adrianreber May 11, 2022 15:24
@rst0git
Copy link
Member Author

rst0git commented May 12, 2022

Thanks, I've rebased this pull request.

Building the criu packages for Ubuntu/Debian fails with:

	mkdir: cannot create directory '/var/lib/criu': Permission denied

This patch updates PLUGINDIR with the value /usr/lib/criu

Fixes: checkpoint-restore#1877

Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
@avagin avagin merged commit 2b3763f into checkpoint-restore:criu-dev May 17, 2022
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