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

nix-gitignore: init at v3.0.0 #46112

Merged
merged 1 commit into from Feb 18, 2019
Merged

nix-gitignore: init at v3.0.0 #46112

merged 1 commit into from Feb 18, 2019

Conversation

siers
Copy link
Member

@siers siers commented Sep 5, 2018 •
edited

Motivation for this change

A user in siers/nix-gitignore#6 requested this to be merged into nixpkgs somehow and I'd gladly try to arrange it.

Questions
  1. I'm not sure whether this is the appropriate directory or the appropriate approach of inclusion.
  2. elvishjerricco on IRC mentioned that I mustn't use the "import from fetchGitHub" approach, so I included it directly.
  3. I have some tests in the project's repo, but I'm not sure whether I should slow down the total running time of all nixpkgs tests with this tiny thing, given that it also spawns a virtual machine for it.
  4. How should I add meta section, if this isn't a derivation?
  5. Anything else to add?

@matthewbauer
Copy link
Member

Could we move this to lib?

@siers
Copy link
Member Author

siers commented Sep 5, 2018

I can move it wherever advised.

afrepues
afrepues suggested changes Sep 6, 2018
View reviewed changes
Copy link
Contributor

@afrepues afrepues left a comment

Choose a reason for hiding this comment

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

Can you please document more what the code does and how it is supposed to be used?

pkgs/build-support/nix-gitignore/default.nix Outdated

let
debug = a: trace a a;
tail = l: elemAt l ((length l) - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

As there is a tail function in builtins already, I recommend that you use a different name to make the code easier to read.

@siers
Copy link
Member Author

siers commented Sep 6, 2018 via email

@siers
Copy link
Member Author

siers commented Sep 18, 2018 •
edited

@afrepues Hi,

in the latest version

  1. I renamed the tail to last as you requested and
  2. added the docs from the repo in a markdown file — that documents "what the code does and how it is supposed to be used". Will that do? :)

@afrepues
Copy link
Contributor

Much better! My thoughts:

Though I can see the usefulness of having a filterSource that uses the patterns from .gitignore files, it is not clear to me when this becomes useful. There is the start of an example in the "Motivation" section, but it is incomplete.

References to the original repository should be removed.

@siers
Copy link
Member Author

siers commented Sep 20, 2018

  1. Are you saying you personally don't understand the value or do you merely think the motivation section doesn't convey it very well?
  2. Should they? Why?

@afrepues
Copy link
Contributor

  1. Are you saying you personally don't understand the value or do you merely think the motivation section doesn't convey it very well?

The documentation doesn't convey it for me. I can see the value of a filter that uses gitignore, but can't think of a use case, and the documentation doesn't give a full one either.

  1. Should they? Why?

Are you talking about the references? If the code is already in this repository, access to this repository alone should suffice to use it. Mainly the examples need to be reworked to show the code already working as living in this repository.

@siers
Copy link
Member Author

siers commented Sep 20, 2018

  1. Would "you can deploy your own website from the development directory that has either tmp/ from rails, _cache/ from hakyll, .stack_work/ from stack or node_modules in it without the need to reclone" be enough? (if mentioned in the README)
  2. Oh, the examples? Yeah, those need be changed for sure. That went right over my head.

@afrepues
Copy link
Contributor

  1. Something on that line would make it much clearer, yes. Ideally, illustrated with working code.

@siers
Copy link
Member Author

siers commented Sep 21, 2018

What about the test code? Are you interested in having that lying around as well?

@siers
Copy link
Member Author

siers commented Sep 22, 2018

  1. The default.nix now throws an error if nix is too old.
  2. The test files have been added.
  3. Motivation section has been redone.

@siers siers changed the title nix-gitignore: init at v1 nix-gitignore: init at v1.0.2 Sep 22, 2018
@siers
Copy link
Member Author

siers commented Sep 27, 2018

Hi, when might you have a chance to take a look at this? :)

@tomberek
Copy link
Contributor

This is very useful. There is often a nice match between what should be ignored by source control and some of the cruft in a directory. This prevents superfluous rebuilds, just because "result" changed. I've been using this for several months.

@siers
Copy link
Member Author

siers commented Oct 2, 2018 •
edited

#nixos 2018-10-01 @zimbatm @grahamc

2018-10-01 15:52:02	 *	zimbatm looking at the PR
2018-10-01 16:06:47	siers	zimbatm, do you think it has a chance of getting merged?
2018-10-01 16:30:55	zimbatm	siers: it's a bit difficult to answer
2018-10-01 16:31:13	zimbatm	I think it would be very useful to have it builtin
2018-10-01 16:31:42	zimbatm	siers: it probably belongs in the lib/ folder
2018-10-01 16:32:37	siers	zimbatm, I can change that. What about the README? Is it necessasry?
2018-10-01 16:32:44	zimbatm	siers: one of the concern would be if this started to be used a lot in nixpkgs as it does too much at evaluation time
2018-10-01 16:33:06	zimbatm	and lib/ is only really a support for nixpkgs
2018-10-01 16:33:23	zimbatm	(unfolding my thought process)
2018-10-01 16:33:57	zimbatm	so I guess having it in build-support would be a signal that it's not meant to be used in nixpkgs directly
2018-10-01 16:34:10	siers	zimbatm, I think it's more for developers and I that's why I didn't put it in lib/. I merely intended it to be more easily accessible.
2018-10-01 16:34:31	zimbatm	the other concern is that it will be harder for you to update it
2018-10-01 16:34:49	zimbatm	you did a lot of good work, it would be a shame if it was lost
2018-10-01 16:35:14	siers	I intended it as a package, but then I couldn't just fetchgit, because you shouldn't import from derivations, so I had to inline all of it.
2018-10-01 16:35:17	zimbatm	yarn2nix that was imported above has this issue, we imported it in nixpkgs but not it's still evolving in the main repo
2018-10-01 16:35:41	zimbatm	yeah
2018-10-01 16:35:52	zimbatm	same issue with yarn2nix
2018-10-01 16:36:07	siers	As long as it does the same thing, I don't think updates would happen too often.
2018-10-01 16:36:31	zimbatm	I think your project should be relatively stable compared to yarn2nix
2018-10-01 16:36:39	zimbatm	so all future development could happen in nixpkgs
2018-10-01 16:37:45	siers	easily
2018-10-01 16:37:56	zimbatm	regarding the README, the best thing to do would be to convert it to the manual
2018-10-01 16:38:24	zimbatm	so move the usage bit to the manual and trim the existing README to testing instructions
2018-10-01 16:38:49	zimbatm	is there any way the tests could be plugged into the rest of the tests?
2018-10-01 16:39:07	siers	Sure, I checked out the tests.
2018-10-01 16:39:55	siers	The nixpkgs tests, that is. Do they spawn a VM for each test? I'm a little concerned about that because VM would probably take longer to spawn than the test time.
2018-10-01 16:41:08	siers	Yeah, I can write instructions for the manual also. I was trying to hint at that and asked about it in the issue, but I didn't get a clear answer. 
2018-10-01 16:41:10	zimbatm	only the NixOS tests are spawning VMs
2018-10-01 16:41:19	zimbatm	the others are running the tests in a derivation
2018-10-01 16:42:04	siers	pkgs/test/ ?
2018-10-01 16:42:10	zimbatm	ask gchristensen, he is the master of manuals :)
2018-10-01 16:42:15	zimbatm	+
2018-10-01 16:42:45	gchristensen	which nixpkgs tests?
2018-10-01 16:42:55	siers	only 27 files there, is that the right directory?
2018-10-01 16:43:19	siers	gchristensen, I would like to get this merged. https://github.com/NixOS/nixpkgs/pull/46112 I have tests for it, but I haven't included them yet.
2018-10-01 16:43:21	gchristensen	I didn't know we had nixpkgs tests :o
2018-10-01 16:43:50	zimbatm	gchristensen: we need that bit moved to the manual: https://github.com/NixOS/nixpkgs/pull/46112/files#diff-7d5b9f9c177be63d2b0b9b46ef3ac0fb
2018-10-01 16:44:12	zimbatm	siers: your package is a bit special, most of them are derivations that have check phases
2018-10-01 16:44:12	gchristensen	oh man
2018-10-01 16:44:39	zimbatm	your is closer to lib/tests
2018-10-01 16:44:50	zimbatm	so pkgs/tests is quite thin but it would fit there
2018-10-01 16:46:33	siers	Okay, I'll take a look at the tests and manuals when I'm not at work.
2018-10-01 16:48:29	gchristensen	siers: so ideally the .md would be rewritten in docbook (see: https://docbook.rocks), and added to nixpkgs/doc/functions.xml if you need help with that conversion, I'm happy to help*

@siers
Copy link
Member Author

siers commented Feb 17, 2019 •
edited

@grahamc @zimbatm

I've added the docs with an include of doc/functions/nix-gitignore.nix in doc/functions.xml. The docs build and I read/adjusted them twice. (this is a funny way of saying it, I hope you'll understand what I meant)

nix-gitignore/default.nix has been copied verbatim with the minor addition of a link to the original repo.

Thanks.

@zimbatm zimbatm merged commit d8a7a01 into NixOS:master Feb 18, 2019
@siers siers changed the title nix-gitignore: init at v1.0.2 nix-gitignore: init at v3.0.0 Feb 18, 2019
@siers
Copy link
Member Author

siers commented Feb 18, 2019 •
edited

I can't believe I finally did it and that finally happened! Thanks @zimbatm!

🎊 🎉

@zimbatm
Copy link
Member

zimbatm commented Feb 18, 2019

🎉 :)

@zimbatm
Copy link
Member

zimbatm commented Feb 18, 2019

thanks for your patience :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afrepues afrepues afrepues requested changes

@zimbatm zimbatm zimbatm approved these changes

Assignees
No one assigned
Labels
8.has: documentation 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@siers @matthewbauer @afrepues @tomberek @zimbatm @GrahamcOfBorg