dmt-core: init at 2.1.0 #320189

jasonodoom · 2024-06-16T04:49:43Z

Description of changes.

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:
- NixOS test(s) (look inside nixos/tests)
- and/or package tests
- or, for functions and "core" functionality, tests in lib/tests or pkgs/test
- made sure NixOS tests are linked to the relevant packages
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.11 Release Notes (or backporting 23.11 and 24.05 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.

fsnkty · 2024-06-17T07:47:52Z

Please revisit Contributing to Nixpkgs, and be more mindful of notifying / requesting review from specific users. see the discourse page or matrix channel for requesting reviews.

fsnkty

the use of an overlay, odd formatting and unexplained shell.nix ? would like justification/explanation.

NobbZ

Basically what @fsnkty said. Just writing text here to suite GHs requirement to give a reason to "Request Changes".

I will also repeat the review related bullet points here, that I gave you in the discord this morning:

Not like that! Please take a look at the contribution guidelines. (linked by @fsnkty)
Don't use overlays
Don't reimport nixpkgs
Don't provide a shell
prefer pkgs/by-name
unless this is really a python package, then use pythonPackages
remember to add yourself to the maintainers.nix file if you haven't already

I also removed Sandro, as I do not think that they will say anything different in the current state of the PR

eclairevoyant

Probably also want to look at other packages built with python and the nixpkgs manual; if dmt-core is not used for its modules, then buildPythonApplication would be preferable (and note some of the other guidelines mentioned in the manual)

jasonodoom · 2024-06-17T15:27:06Z

Thanks for the feedback everyone. I will rewrite this. It was my understanding that I needed an overlay but it doesn't seem to be necessary here.

jasonodoom · 2024-06-17T17:17:19Z

the use of an overlay, odd formatting and unexplained shell.nix ? would like justification/explanation.

The formatting is the result of nixfmt shell.nix should have been omitted. The reason for overlay is because I needed to pull two dependencies which have not been packaged but I realize now this is not necessary and there is an alternative method.

pkgs/by-name/dm/dmt-core/package.nix

eclairevoyant · 2024-06-27T07:17:13Z

pkgs/by-name/dm/dmt-core/package.nix

+{ lib, buildPythonApplication, fetchurl, fetchPypi, pythonPackages }:
+
+let
+ reprint = buildPythonApplication rec {


is this a python module? and in any case, why not package it separately?

It is. Which is why my initial commit was an overlay. It's used as a dependency. I don't see it as necessary for the time being.

Never said anything about an overlay, overlays are for when you can't just modify nixpkgs itself - but we are in the nixpkgs repo, we can modify nixpkgs itself.

I'm saying these are probably best packaged in separate files.

And as they are modules, not applications, we'd not use pkgs/by-name for those, nor buildPythonApplication.

I'm receiving conflicting feedback in that case because my initial PR didn't use pkgs/by-name. The description even notes that it's a lib. I'm not sure if anyone is reading it. I'll make those modifications.

There's no conflicting feedback here. Nobbz said

prefer pkgs/by-name
unless this is really a python package, then use pythonPackages

The phrasing to use pythonPackages was incorrect, it should've been python3Packages. Other than that, we are in agreement; if it's used for its modules, as I said earlier, then it stays in python-modules. If it's simply a binary, then it goes in pkgs/by-name.

So you need dmt-core in pkgs/by-name using buildPythonApplication, while reprint and verilogae go in python-modules using buildPythonPackage, if I understood the purpose of these packages correctly.

The phrasing to use pythonPackages was incorrect, it should've been python3Packages

Yeah, my fault, pythonPackages belong in python2-modules and python3Packages in python-modules, I hate whoever came up with that…

Though in general I used "pythonPackages" as the generic name for "the relevant pythonXPackages"

pkgs/by-name/dm/dmt-core/package.nix

vigress8 · 2024-06-27T07:39:02Z

pkgs/by-name/dm/dmt-core/package.nix

+ reprint = buildPythonApplication rec {
+ pname = "reprint";
+ version = "0.6.0";
+ src = fetchurl {


Prefer fetchPypi, and use hash instead of sha256

I've had trouble using fetchPypi It's what I used earlier but lots of 404 despite following the recommendations in the documentation. Also, DMT_core is the package name as formatted in PyPi. dmt-core will fail.

pkgs/by-name/dm/dmt-core/package.nix

vigress8 · 2024-06-27T07:43:58Z

pkgs/by-name/dm/dmt-core/package.nix

+ verilogae = buildPythonApplication rec {
+ pname = "verilogae";
+ version = "1.0.0";
+ src = fetchurl {


Same as reprint

vigress8 · 2024-06-27T07:46:49Z

Did you try building this on your own machine before pushing? Because it fails for at least two immediate reasons - buildPythonApplication is not at the top-level, and pythonPackages uses python2 which is marked as insecure

jasonodoom · 2024-06-27T08:03:02Z

Did you try building this on your own machine before pushing? Because it fails for at least two immediate reasons - buildPythonApplication is not at the top-level, and pythonPackages uses python2 which is marked as insecure

Yes I was able to build without issues. Good to know pythonPackages uses python2. Nobbz told me to use that. Hence, why I made the modification. But I wasn't aware of that detail until now.

eclairevoyant · 2024-06-27T13:02:56Z

What exact command did you use to test building?
nix-build -A dmt-core wouldn't work as-is.

jasonodoom · 2024-06-27T13:53:54Z

What exact command did you use to test building?

nix-build -A dmt-core wouldn't work as-is.

nix-build -E 'with import <nixpkgs> {}; pkgs.python3Packages.callPackage ./package.nix {}'

eclairevoyant · 2024-06-27T14:44:33Z

What exact command did you use to test building?
nix-build -A dmt-core wouldn't work as-is.

nix-build -E 'with import <nixpkgs> {}; pkgs.python3Packages.callPackage ./package.nix {}'

Don't callPackage it manually, the whole point of having nixpkgs is that people don't have to run that themselves. Please use the command I provided.

eclairevoyant · 2024-06-27T14:46:45Z

Standalone packages are built using pkgs.callPackage. Use that for testing

No, pkgs/by-name already handles callPackage-ing, and outside of that, the callPackage-ing is done in the appropriate pkgs/top-level/*.nix files. We really need to just stick to using nix-build -A <attribute> here.

Also, I'd appreciate not providing misleading and duplicate feedback on the PR, as this will cause more confusion and frustration.

Co-authored-by: V. <[email protected]> Co-authored-by: éclairevoyant <[email protected]>

NobbZ

Formalizing my response from the discord PR thread:

Please extract the dependencies into their own (python) packages
Please make sure you are following the contribution guideline, especially the part about how commits should be seperated and named. Squash and force push to fix that.

NobbZ · 2024-06-29T09:05:54Z

And… It just happened that I looked at the upstream repo, to figure out what the main scripts name was, also built and tried to check the build output…

There doesn't seem to be any mention of a runnable program in the repo, nor is one in the build output.

This seem to be a library not an application, please package this as a python module, not at the top-level!

jasonodoom · 2024-06-29T15:12:01Z

And… It just happened that I looked at the upstream repo, to figure out what the main scripts name was, also built and tried to check the build output…

There doesn't seem to be any mention of a runnable program in the repo, nor is one in the build output.

This seem to be a library not an application, please package this as a python module, not at the top-level!

Yes, this is a library. That's why I was confused about the request to move it under by-name/ It's in the description. 😭

jasonodoom assigned SuperSandro2000 and unassigned SuperSandro2000 Jun 16, 2024

jasonodoom requested a review from SuperSandro2000 June 16, 2024 04:53

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jun 16, 2024

jasonodoom requested a review from NobbZ June 16, 2024 21:04

fsnkty suggested changes Jun 17, 2024

View reviewed changes

NobbZ requested changes Jun 17, 2024

View reviewed changes

NobbZ removed the request for review from SuperSandro2000 June 17, 2024 11:34

eclairevoyant reviewed Jun 17, 2024

View reviewed changes

vigress8 self-assigned this Jun 21, 2024

jasonodoom force-pushed the dmt-core branch 3 times, most recently from bf87ec4 to 5572f2d Compare June 27, 2024 06:26

dmt-core: init at 2.1.0

d2e6a7a

jasonodoom force-pushed the dmt-core branch from 3a46e10 to d2e6a7a Compare June 27, 2024 06:31

jasonodoom requested review from NobbZ and eclairevoyant June 27, 2024 06:52

eclairevoyant reviewed Jun 27, 2024

View reviewed changes

vigress8 suggested changes Jun 27, 2024

View reviewed changes

Apply suggestions from code review

57bb99a

Co-authored-by: V. <[email protected]> Co-authored-by: éclairevoyant <[email protected]>

ofborg bot added 8.has: package (new) 11.by: package-maintainer 10.rebuild-linux: 1-10 10.rebuild-linux: 1 and removed 10.rebuild-linux: 0 labels Jun 28, 2024

dmt-core: init at 2.1.0

7798c65

jasonodoom requested a review from eclairevoyant June 28, 2024 03:06

Set different pname for fetcher

58dc837

jasonodoom requested a review from vigress8 June 28, 2024 13:12

jasonodoom mentioned this pull request Jun 28, 2024

dmt-core: init at 2.1.0 ngi-nix/ngipkgs#286

Closed

NobbZ requested changes Jun 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dmt-core: init at 2.1.0 #320189

dmt-core: init at 2.1.0 #320189

jasonodoom commented Jun 16, 2024

fsnkty commented Jun 17, 2024

fsnkty left a comment

NobbZ left a comment •

edited

Loading

eclairevoyant left a comment •

edited

Loading

jasonodoom commented Jun 17, 2024

jasonodoom commented Jun 17, 2024

eclairevoyant Jun 27, 2024

jasonodoom Jun 27, 2024

eclairevoyant Jun 27, 2024 •

edited

Loading

jasonodoom Jun 27, 2024

eclairevoyant Jun 27, 2024

NobbZ Jun 27, 2024 •

edited

Loading

vigress8 Jun 27, 2024

jasonodoom Jun 28, 2024

vigress8 Jun 27, 2024

vigress8 commented Jun 27, 2024

jasonodoom commented Jun 27, 2024

eclairevoyant commented Jun 27, 2024 •

edited

Loading

jasonodoom commented Jun 27, 2024

eclairevoyant commented Jun 27, 2024

eclairevoyant commented Jun 27, 2024 •

edited

Loading

NobbZ left a comment

NobbZ commented Jun 29, 2024

jasonodoom commented Jun 29, 2024

dmt-core: init at 2.1.0 #320189

Are you sure you want to change the base?

dmt-core: init at 2.1.0 #320189

Conversation

jasonodoom commented Jun 16, 2024

Description of changes.

Things done

fsnkty commented Jun 17, 2024

fsnkty left a comment

Choose a reason for hiding this comment

NobbZ left a comment • edited Loading

Choose a reason for hiding this comment

eclairevoyant left a comment • edited Loading

Choose a reason for hiding this comment

jasonodoom commented Jun 17, 2024

jasonodoom commented Jun 17, 2024

eclairevoyant Jun 27, 2024

Choose a reason for hiding this comment

jasonodoom Jun 27, 2024

Choose a reason for hiding this comment

eclairevoyant Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

jasonodoom Jun 27, 2024

Choose a reason for hiding this comment

eclairevoyant Jun 27, 2024

Choose a reason for hiding this comment

NobbZ Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

vigress8 Jun 27, 2024

Choose a reason for hiding this comment

jasonodoom Jun 28, 2024

Choose a reason for hiding this comment

vigress8 Jun 27, 2024

Choose a reason for hiding this comment

vigress8 commented Jun 27, 2024

jasonodoom commented Jun 27, 2024

eclairevoyant commented Jun 27, 2024 • edited Loading

jasonodoom commented Jun 27, 2024

eclairevoyant commented Jun 27, 2024

eclairevoyant commented Jun 27, 2024 • edited Loading

NobbZ left a comment

Choose a reason for hiding this comment

NobbZ commented Jun 29, 2024

jasonodoom commented Jun 29, 2024

NobbZ left a comment •

edited

Loading

eclairevoyant left a comment •

edited

Loading

eclairevoyant Jun 27, 2024 •

edited

Loading

NobbZ Jun 27, 2024 •

edited

Loading

eclairevoyant commented Jun 27, 2024 •

edited

Loading

eclairevoyant commented Jun 27, 2024 •

edited

Loading