Bug 255670 - Mk/Keywords/sample.ucl: Deleting any configuration files automatically is a bug
Summary: Mk/Keywords/sample.ucl: Deleting any configuration files automatically is a bug
Status: Open
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Ports Framework (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Port Management Team
URL: https://jetcafe.org/dave/pkgissue.html
Keywords: needs-qa
Depends on:
Blocks:
 
Reported: 2021-05-06 20:44 UTC by dave
Modified: 2021-05-27 02:53 UTC (History)
4 users (show)

See Also:
koobs: maintainer-feedback? (portmgr)


Attachments
Proposed change to the sample Keyword's behavior (475 bytes, patch)
2021-05-08 23:46 UTC, dave
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description dave 2021-05-06 20:44:14 UTC
I contend that the current behavior of pre-deinstall-lua is a bug. The fix is changing this:

  if pkg.filecmp(sample_file, target_file) == 0 then
    os.remove(target_file)
  else
    pkg.print_msg("You may need to manually remove " .. target_file .. " if it is no longer needed.")
  end

to this:

    pkg.print_msg("You may need to manually remove " .. target_file .. " if it is no longer needed.")

My rationale for this is here:

    https://www.jetcafe.org/dave/pkgissue.html

(I tried to use the correct component, but bugzilla will not let me do that for some reason.)
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2021-05-07 04:10:56 UTC
Thank you for the report and patch Dave.

Can you include your proposed change as an attachment please. 

Apologies for the current inability to set the correct component.

Note also that the URL provided is currently 404, I highly recommend attaching any supporting documentation to this issue, which ensures beyond being available, that relevent information does not stale out or bitrot over time, as tensds to be the case with external references
Comment 2 dave 2021-05-08 23:46:16 UTC
Created attachment 224781 [details]
Proposed change to the sample Keyword's behavior
Comment 3 dave 2021-05-08 23:51:45 UTC
First of all, apologies about the 404, this is fixed (redirect issue). 

Next, jetcafe.org has been around since 1995 so I'm comfortable with leaving the rationale there. :)

Finally, I've attached a patch as requested.
Comment 4 Adam Weinberger freebsd_committer 2021-05-18 15:32:08 UTC
I'm not fully understanding the problem. Your website talks about nginx breaking because the configuration files are removed during the removal step of a `pkg upgrade`, but they should be recreated immediately when the package is upgraded/reinstalled.

The only circumstance when nginx should break is when nginx is installed but the configuration files aren't. That seems to be what you're talking about, but I'm unclear how that is happening.

Can you provide more detail?
Comment 5 dave 2021-05-18 17:46:04 UTC
(In reply to Adam Weinberger from comment #4)

> they should be recreated immediately when the package is upgraded/reinstalled

I have not observed this behavior consistently. Instead, with recent upgrades (1.14.6 -> 1.16.3) I've observed that in some cases the -dist files are not automatically installed (leaving no file in it's place), and in others they are. Note that this will likely be a separate (but related) issue, as I do not have enough actionable data to file a bug report yet (nor do I know if the inconsistent behavior is, in fact, a bug or something else). 

Still, this technically unrelated behavior is, in fact, the stimulus for my change request. Not installing -dist files is somewhat expected given that they are -dist files, but not expected by the person who wrote the nginx.conf file which includes them. I'm unclear why the current behavior of pre-deinstall-lua contains any notion of os.remove() ... I find this to be an unsafe practice.

As far as I can tell, the mechanism in question is with the @sample keyword in the pkg-plist file. However, maybe this mechanism is insufficient or perhaps I am naive about my understanding of this idea? There are two cases I can see happening:

1) The port needs those files installed even if they are samples

2) The port does not need those files installed. 

The sample keyword appears to assume (2), not (1). Neither case implies deleting what is already there. 

Do you know of a way to increase verbosity of pkg in a way that sheds more light on this and related issues?
Comment 6 Tatsuki Makino 2021-05-18 22:12:12 UTC
*.sample file will be registered with the hash.
Editing it will cause pkg check -s to report the problem.
* copied from *.sample will be used as the configuration file, but if the configuration file was not edited, you can delete it at the same time as uninstalling.
That's what @sample is for, isn't it?
Comment 7 dave 2021-05-19 20:26:24 UTC
> if the configuration file was not edited, you can delete it at the same time as uninstalling.

You are able to, yes. Whether it -should- be done is the subject of my bug report. We are observing the deletion of an unedited configuration file but not a subsequent replacement where replacement is necessary (e.g. the nginx port). 

I claim we don't need to delete this unedited file and that it's safer not to delete it in case issues like this arise. 

I do recognize (and should clarify if I haven't already) that there is a related issue with pkg deleting in-place configuration files but I don't yet have solid data so I can report it or even see if this is my issue and not FreeBSD's. This particular bug report is not intended to address the related issue.
Comment 8 Adam Weinberger freebsd_committer 2021-05-20 01:57:30 UTC
OK, it sounds like the real issue here is that `pkg upgrade` is removing @sample files and not always replacing them.

Removing the file isn’t the problem, so while this patch would work around it, it doesn’t address the underlying issue. We need to identify how and when the files are removed and not recreated.

I haven’t seen this on any of my systems. I’m curious whether others are seeing this as well. Can you provide some more info about your environment? Are you building the packages yourself, or using FreeBSD-provided packages? How are you upgrading them? Does this happen on multiple machines, or just on one?
Comment 9 dave 2021-05-27 02:53:48 UTC
(In reply to Adam Weinberger from comment #8)
> Removing the file isn’t the problem, so while this patch would work around it,
> it doesn’t address the underlying issue. We need to identify how and when the 
> files are removed and not recreated.

I agree that the underlying issue is not addressed. I would still support this patch being included as a policy idea. 

We have found that actually removing the -dist files prior to the upgrade consistently prevents the deletion of the @sample files, regardless of whether they have been edited or not.

> Can you provide some more info about your environment? 

Yes. I build packages with poudriere:

# poudriere version
3.3.2-10-g8e54ca5a

for all my target sites. Each target site has different options directories and different package repositories. These streams are not crossed; upgrading a target site always uses the same options directories, package repositories, and build configuration. When I do these builds, I generally do them against the quarterly ports tree build I've picked at the time. Occasionally I cherry pick some ports from HEAD which have needed security upgrades. 

I use pkg(8) to manage package installations. I've been doing this for several years with no obvious issues. Each package upgrade goes like this: 

1) Alter the repository configuration file to point to the new package repo
2) Upgrade pkg first with "pkg upgrade pkg"
3) Then let pkg have it's way with "pkg upgrade"

This has always worked before and until this particular upgrade @sample files were not deleted. 

I did not notice the first instance of file deletion until I discovered that the file fastcgi_params was missing and nginx would not start. It was curious to me because I never edited that file (most of my nginx sites are templated and explicitly configured after we include this file). 

I did not discover this was pkg until -at a different site- the same thing happened. 

Let me know if you need more information.