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

Add warning for missing maintainer scripts files #70

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ltabis
Copy link

@ltabis ltabis commented Jan 22, 2023

Regarding #69

When the maintainer-scripts field is set and a script is not found, cargo deb will emit a warning telling the user that it will supply the script itself.

This generates the following logs for the vsmtp project: (which provides postinst, postrm and prerm)

info: Determining augmentations needed for systemd unit vsmtp.service
info: Maintainer script postinst will be augmented with autoscript postinst-systemd-enable
info: Maintainer script postrm will be augmented with autoscript postrm-systemd
info: Maintainer script postinst will be augmented with autoscript postinst-systemd-restartnostart
info: Maintainer script prerm will be augmented with autoscript prerm-systemd-restart
info: Maintainer script postrm will be augmented with autoscript postrm-systemd-reload-only
info: Augmenting maintainer script /vSMTP/tools/install/deb/postinst
info: Augmenting maintainer script /vSMTP/tools/install/deb/prerm
info: Augmenting maintainer script /vSMTP/tools/install/deb/postrm
+warning: maintainer script config was not found at location "/vSMTP/tools/install/deb", generating defaults instead.
+warning: maintainer script preinst was not found at location "/vSMTP/tools/install/deb", generating defaults instead.
+warning: maintainer script templates was not found at location "/vSMTP/tools/install/deb", generating defaults instead.

I failed to run the test suite on the following tests because there is a missing test-resources/testroot/testchild/debian directory.

control::tests::generate_scripts_archives_user_supplied_maintainer_scripts_in_root_package
control::tests::generate_scripts_archives_user_supplied_maintainer_scripts_in_workspace_package
control::tests::generate_scripts_augments_maintainer_scripts_for_unit_in_root_package
control::tests::generate_scripts_augments_maintainer_scripts_for_unit_in_workspace_package
control::tests::generate_scripts_generates_missing_maintainer_scripts_for_unit_in_root_package
control::tests::generate_scripts_generates_missing_maintainer_scripts_for_unit_in_workspace_package

How can I get it ?

@kornelski
Copy link
Owner

The tests are they way they are. If they don't run out of the box, figure out whether that's an incompatible change or wrong tests.

@ximon18
Copy link
Contributor

ximon18 commented Jan 23, 2023

I think that the warning isn't strictly correct because it doesn't generate a default script in the absence of a particular script, this isn't even a reason to warn is it? I can imagine it being useful to warn if zero scripts were found, rather than just some. (update: What I was trying to say is that scripts are not required to exist, it is valid to provide only some rather than all possible scripts)

When I wrote the systemd unit maintainer script fragment injection stuff at the time cargo-deb had no concept of debug logging, but I wanted and still think there would be value in adding "debug" level logging to show where the logic is looking for and failing to find files, to help with the case that it doesn't do as you expect.

@kornelski kornelski force-pushed the main branch 6 times, most recently from d484750 to 5f87af3 Compare March 7, 2023 17:58
@@ -64,7 +64,7 @@ impl<'l, W: Write> ControlArchiveBuilder<'l, W> {
/// should be inserted.
fn generate_scripts(&mut self, option: &Config) -> CDResult<()> {
if let Some(ref maintainer_scripts_dir) = option.maintainer_scripts {
let maintainer_scripts_dir = option.package_manifest_dir.as_path().join(maintainer_scripts_dir);
let maintainer_scripts_dir = option.package_manifest_dir.as_path().join(maintainer_scripts_dir).canonicalize()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

When maintainer_scripts is set to a directory that does not exist this line caused the following output for me:

cargo-deb: I/O error: No such file or directory (os error 2)
  because: No such file or directory (os error 2)

I had no idea which part of the code was throwing it or which directory it was looking for.

@janrueth
Copy link

Just wanted to encourage to make this a hard failure instead of a warning.

After #11 was merged and our tooling picked those changes up (which seems to have taken some time :)) we discovered that the maintainer scripts were not included anymore as we were still specifying paths relative to the workspace.

I think cargo-deb should not only emit a warning but an error and stop processing when this happens (of course explaining nicely what the problem is :)). I'm not sure we would have noticed a warning among the tons of output from CI.

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