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

cope: remove #303596

Merged
merged 1 commit into from
Apr 12, 2024
Merged

cope: remove #303596

merged 1 commit into from
Apr 12, 2024

Conversation

NobbZ
Copy link
Contributor

@NobbZ NobbZ commented Apr 12, 2024

Description of changes

cope upstream is a bunch of wrapper script, which require the same named binary to exist somewhere else in PATH.

This design barely works with nixpkgs, and fixing this would probably require a good amount of patching. It is also unclear if the nearly 10 year old wrappers still work for the modern variants of the wrapped tools.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Copy link
Contributor

@msanft msanft left a comment

Choose a reason for hiding this comment

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

Juding from how the upstream looks like, and a best-effort search on how many dependants this package has, removing it from nixpkgs makes a lot of sense to me.

@ofborg ofborg bot added 8.has: clean-up 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Apr 12, 2024
Copy link
Member

@pSub pSub left a comment

Choose a reason for hiding this comment

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

+1 on the approval of @msanft

Copy link
Contributor

@eclairevoyant eclairevoyant left a comment

Choose a reason for hiding this comment

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

Even expanding the search, seems most of the usages of "cope" are in comments. Some of them are even commenting out cope from their package lists.

And maintaining patches for a 10+ year old codebase is nontrivial.

@NobbZ
Copy link
Contributor Author

NobbZ commented Apr 12, 2024

nixpkgs-review does cause astonishing 0 rebuilds, so I doubt there are any dependees.

@msanft
Copy link
Contributor

msanft commented Apr 12, 2024

nixpkgs-review does cause astonishing 0 rebuilds, so I doubt there are any dependees.

With the comment above, I referred to out-of-tree consumers of nixpkgs using this package. However, no in-tree dependants are good as well :D

@leona-ya leona-ya merged commit 0921e05 into NixOS:master Apr 12, 2024
22 checks passed
@deftdawg
Copy link
Contributor

deftdawg commented Apr 15, 2024

Cope actually still works pretty well if it's patched for the warnings, I was running the warnings patch plus my own to make it use nixos's command-not-found.

image

Applied this overlay for my version @ https://github.com/deftdawg/cope

nixpkgs.overlays = [
    # Fix Cope using DeftDawg Version
		(final: prev: {
			cope = prev.cope.overrideAttrs (oldAttrs: {
				version = "unstable-2024-03-27";
        src = pkgs.fetchFromGitHub {
          owner = "deftdawg";
          repo = "cope";
          rev = "ad0c1ebec5684f5ec3e8becf348414292c489175";
          sha256 = "sha256-LMAir7tUkjHtKz+KME/Raa9QHGN1g0bzr56fNxfURQY=";
        };
	});
    })
 ];

Bummer to see it go from the official repo, I guess I'll have to roll it into a flake.

@NobbZ
Copy link
Contributor Author

NobbZ commented Apr 15, 2024

Feel free to PR your changes here and add yourself as a maintainer then.

I only skimmed copes sources and considered it worth a removal after having people run against that wall for about the 100th time in the last 12 months, just because cope has been the first result on the web search, CLI search, command-not-found, or nix-index for a given term.

Keeping it the way it was, seemed to be more troublesome than removing it.

Though I really do not know what warnings you are talking about.

I only have seen hard errors with it…

Running a cope-wrapped script always resulted in an error that exactly that command wasn't found.

@deftdawg
Copy link
Contributor

Closing #274361 which was to fix those warnings (opened in Dec'23) and opening #304918 to restore cope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: clean-up 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants