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.

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

md2gemini: init at 1.8.1 #117917

Merged
merged 4 commits into from
Apr 8, 2021
Merged

md2gemini: init at 1.8.1 #117917

merged 4 commits into from
Apr 8, 2021

Conversation

KAction
Copy link
Contributor

@KAction KAction commented Mar 29, 2021 •
edited
Loading

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

SuperSandro2000
SuperSandro2000 requested changes Mar 29, 2021
View reviewed changes
pkgs/top-level/python-packages.nix Outdated
mistune_2 = callPackage ../development/python-modules/mistune {
source-info = {
version = "2.0.0a4";
sha256 = "0i6cblmjl58kdmaa21xm0l1ls0kvjpfy45sf73fw3ws6305f628k";
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be clearer if we converted the current default into a generic nix and created a 0.8.nix and a 2.nix file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did it same way as postgresql do.

pkgs/development/python-modules/md2gemini/default.nix Show resolved Hide resolved
pkgs/development/python-modules/md2gemini/default.nix Outdated
Comment on lines 6 to 8

propagatedBuildInputs = [ mistune_2 cjkwrap wcwidth ];
checkInputs = [ pytest ];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
propagatedBuildInputs = [ mistune_2 cjkwrap wcwidth ];
checkInputs = [ pytest ];

pkgs/development/python-modules/md2gemini/default.nix Show resolved Hide resolved
pkgs/development/python-modules/md2gemini/default.nix Outdated
@@ -0,0 +1,20 @@
{ lib, buildPythonPackage, fetchPypi, mistune_2, cjkwrap, wcwidth, pytest }:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ lib, buildPythonPackage, fetchPypi, mistune_2, cjkwrap, wcwidth, pytest }:
{ lib, buildPythonPackage, fetchPypi, mistune_2, cjkwrap, wcwidth, pytestCheckHook }:

pkgs/development/python-modules/cjkwrap/default.nix Show resolved Hide resolved
pkgs/development/python-modules/cjkwrap/default.nix Show resolved Hide resolved
pkgs/development/python-modules/cjkwrap/default.nix Show resolved Hide resolved
SuperSandro2000
SuperSandro2000 reviewed Mar 29, 2021
View reviewed changes
pkgs/top-level/all-packages.nix Outdated
@@ -668,7 +668,7 @@ in

act = callPackage ../development/tools/misc/act { };

actdiag = with python3.pkgs; toPythonApplication actdiag;
actdiag = with python3; toPythonApplication actdiag;
Copy link
Member

Choose a reason for hiding this comment

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

That causes an eval error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does not belong here, removed.

@KAction
Copy link
Contributor Author

KAction commented Apr 2, 2021

Looks like I applied all suggestions.

@SuperSandro2000
Copy link
Member

@ofborg eval

@KAction
Copy link
Contributor Author

KAction commented Apr 3, 2021

Okay, now @ofborg is happy.

SuperSandro2000
SuperSandro2000 reviewed Apr 5, 2021
View reviewed changes
pkgs/development/python-modules/mistune/common.nix

meta = with lib; {
description = "The fastest markdown parser in pure Python";
homepage = "https://github.com/lepture/mistune";
Copy link
Member

Choose a reason for hiding this comment

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

Missing maintainer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was this way before.

@KAction
Copy link
Contributor Author

KAction commented Apr 6, 2021

Okay, I added pythonImportsCheck to mistune, but I'd rather not assume maintainership of every indirect dependency I touch. Wh

@SuperSandro2000
Copy link
Member

Okay, I added pythonImportsCheck to mistune, but I'd rather not assume maintainership of every indirect dependency I touch. Wh

The problem is that if your dependencies are not maintained they are potentially in a worse state but it seems like mistune is used in a lot more packages.

I think adding the imports check to mistune triggered the mass rebuild. We really want to have one if there are not tests because otherwise the package can silently break. I think the best solution would be to merge this PR into staging because if we split the change out we force a merge conflict which someone would need to resolve. Let me know if thats a problem for you.

SuperSandro2000
SuperSandro2000 approved these changes Apr 7, 2021
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

other than that the ordering could be a bit more standard LGTM if rebased to staging

SuperSandro2000
SuperSandro2000 approved these changes Apr 7, 2021
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

other than that the ordering could be a bit more standard LGTM if rebased to staging

@KAction
Copy link
Contributor Author

KAction commented Apr 7, 2021

Yes, sure. Let's merge it into staging.

@SuperSandro2000
Copy link
Member

/rebase staging

@github-actions github-actions bot changed the base branch from master to staging April 8, 2021 00:09
@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2021

Rebased, please reopen the pull request to restart CI

@github-actions github-actions bot closed this Apr 8, 2021
@SuperSandro2000 SuperSandro2000 merged commit 97490e6 into NixOS:staging Apr 8, 2021
@dotlambda dotlambda mentioned this pull request Aug 20, 2022
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 501-1000 10.rebuild-darwin: 501+ 10.rebuild-linux: 501-1000 10.rebuild-linux: 501+ 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KAction @SuperSandro2000