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

ddev: init at 1.21.4 #210579

Closed
wants to merge 3 commits into from
Closed

ddev: init at 1.21.4 #210579

wants to merge 3 commits into from

Conversation

jgonyea
Copy link

@jgonyea jgonyea commented Jan 13, 2023

Description of changes

This PR is for adding a new package: ddev
"Docker-based local PHP + Node.js web development environments"

Homepage: https://ddev.readthedocs.io/en/stable/
Source: https://github.com/drud/ddev

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@drupol
Copy link
Contributor

drupol commented Jan 14, 2023

Hello,

Since ddev rely on Docker, I believe you should add it as a dependency, or else it won't work at all.

@jgonyea
Copy link
Author

jgonyea commented Jan 14, 2023

@drupol , that makes sense. I added both docker and docker-compose.

@jgonyea
Copy link
Author

jgonyea commented Jan 17, 2023

@drupol , is there anything else I should do on this PR?

Thanks in advance.

@drupol
Copy link
Contributor

drupol commented Jan 17, 2023

Hi,

I don't know since I never used ddev.

Leave it open, someone else will most probably give some feedback at some point.

@JulienMalka
Copy link
Member

built correctly on x86_64-linux, binary seems to be working although I never used ddev.

Copy link
Member

@JulienMalka JulienMalka left a comment

Choose a reason for hiding this comment

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

Could you squash your commits into 2 commits:

  • maintainers: add jgonyea
  • ddev: init at 1.21.4

Thanks!


src = fetchFromGitHub {
owner = "drud";
repo = "ddev";
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
repo = "ddev";
repo = pname;

@jgonyea
Copy link
Author

jgonyea commented Jan 18, 2023

Could you squash your commits into 2 commits:

@JulienMalka
I commingled the new author and initial ddev project in my first commit. Should I roll it all back and submit more cleanly?

@JulienMalka
Copy link
Member

JulienMalka commented Jan 18, 2023

Could you squash your commits into 2 commits:

@JulienMalka I commingled the new author and initial ddev project in my first commit. Should I roll it all back and submit more cleanly?

You probably want to do something like git reset HEAD~4, then commit again in the right order and by splitting the commits like explained, then push again using git push --force

@jgonyea
Copy link
Author

jgonyea commented Jan 19, 2023

@JulienMalka, thanks. I force pushed those requested changes. Please let me know if there's anything else I can do.

Copy link
Member

@JulienMalka JulienMalka left a comment

Choose a reason for hiding this comment

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

lgtm

@jgonyea
Copy link
Author

jgonyea commented Feb 9, 2023

Anything more that I should do on this for merging? This is my first submission, so I don't know the process.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/831

, ddev }:

buildGoPackage rec {
pname = "ddev";
Copy link
Member

Choose a reason for hiding this comment

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

Please use buildGoModule

Comment on lines +15 to +28
let
pname = "ddev";
version = "1.21.6";
in

stdenv.mkDerivation {
name = pname;
version = version;
src = fetchFromGitHub {
owner = "ddev";
repo = pname;
rev = "v${version}";
sha256 = "sha256-Wjg0Yxo/ulY6R6hhUMFvNZUSwpXENmAHU7GPbgdw7tw=";
};
Copy link
Member

@SuperSandro2000 SuperSandro2000 May 3, 2023

Choose a reason for hiding this comment

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

Suggested change
let
pname = "ddev";
version = "1.21.6";
in
stdenv.mkDerivation {
name = pname;
version = version;
src = fetchFromGitHub {
owner = "ddev";
repo = pname;
rev = "v${version}";
sha256 = "sha256-Wjg0Yxo/ulY6R6hhUMFvNZUSwpXENmAHU7GPbgdw7tw=";
};
stdenv.mkDerivation rec {
pname = "ddev";
version = "1.21.6";
src = fetchFromGitHub {
owner = "ddev";
repo = "ddev";
rev = "v${version}";
sha256 = "sha256-Wjg0Yxo/ulY6R6hhUMFvNZUSwpXENmAHU7GPbgdw7tw=";
};

@SuperSandro2000
Copy link
Member

Please follow the contributing guide when naming your commits.

rev = "v${version}";
sha256 = "sha256-Wjg0Yxo/ulY6R6hhUMFvNZUSwpXENmAHU7GPbgdw7tw=";
};
buildInputs = [ bash curl git go ];
Copy link
Member

Choose a reason for hiding this comment

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

Please patch that the build requires a git clone. We can replace the values returned by git with dummy values or version.

build flags: SHELL=/nix/store/zlf0f88vj30sc7567b80l52d19pbdmy2-bash-5.2-p15/bin/bash build
fatal: not a git repository (or any of the parent directories): .git
make: *** [Makefile:96: .gotmp/bin/darwin_arm64/mkcert] Error 6

Also please split the packages for cross compilation into nativeBuildInputs and buildInputs.

@jgonyea
Copy link
Author

jgonyea commented May 8, 2023

Sorry, I didn't mean to leave this PR open. I need to rework this. Attempting to use the upstream Makefile fails due to the Makefile downloading external files (binary for mkcert).

Withdrawing PR.

@gbytedev
Copy link

@jgonyea I rely on DDEV and it keeps me from switching to NixOS on the desktop. I'd be happy to test the functionality as soon as it is testable. Thanks for your contribution.

@rfay
Copy link

rfay commented Aug 9, 2023

@jgonyea I'm happy to help you get this going again.

@gbytedev
Copy link

@rfay I created a buildable ddev package and put it on the NUR for the time being. I spoke to @jgonyea and the plan is for me to create a new RP from that soon.

@rfay
Copy link

rfay commented Aug 10, 2023

Nice! Is there a way to get notifications for that? How does it get updated (since we're at v1.22.1?)

@gbytedev
Copy link

Nice! Is there a way to get notifications for that? How does it get updated (since we're at v1.22.1?)

Use the configuration from the blog post and the package will update along with the system. In terms of it being up to date in the NUR, it seems I am responsible for now. I subscribe to ddev and will be pushing relatively timely updates.

I believe you can subscribe to my repository directly to get updates on the package - this is where the NUR runner is pulling changes from.

@drupol
Copy link
Contributor

drupol commented Aug 10, 2023

Sorry for the interruption but... why is ddev not in nixpkgs ?

@gbytedev
Copy link

Sorry for the interruption but... why is ddev not in nixpkgs ?

I mentioned earlier that I intend to create a PR very soon. Still testing things ATM.

@rfay
Copy link

rfay commented Aug 10, 2023

Subscribed, thanks.

@gbytedev
Copy link

Subscribed, thanks.

One more thing, keep in mind the NUR runner is active once a day or so so you won't see changes right after I push an update to my repo. :/

@star-szr
Copy link
Contributor

star-szr commented Aug 18, 2023

I submitted #250017 which is a lightly edited version of what I have been using and maintaining personally since December 2022. I would love some feedback and testing there. I just found this issue today after thinking for a while that I should submit my package to nixpkgs.

@gbytedev I'd be happy to add you as a maintainer if you're interested!

@gbytedev
Copy link

@star-szr Feel free add me; I'm happy to deprecate my NUR package and support the upstream one as it has just been merged apparently. 🙌

@star-szr
Copy link
Contributor

@gbytedev I don't see you in https://raw.githubusercontent.com/NixOS/nixpkgs/master/maintainers/maintainer-list.nix, but if you'd like to help maintain the newly merged package, please open a PR to add yourself to the maintainer list and to the ddev package and I'd be happy to give it a +1. Thanks!

@jgonyea
Copy link
Author

jgonyea commented Aug 21, 2023

I have nothing to add, except for thanks for all the work on getting this package working. I lacked the skills necessary to make this a proper release. I'll be glad to help test once it goes live

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants