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

imag: init at 0.10.0 #77362

Closed
wants to merge 1 commit into from
Closed

imag: init at 0.10.0 #77362

wants to merge 1 commit into from

Conversation

@Br1ght0ne
Copy link
Contributor

@Br1ght0ne Br1ght0ne commented Jan 9, 2020

Motivation for this change

https://imag-pim.org/blog/2019/12/24/imag-0-10-0/
https://github.com/matthiasbeyer/imag/releases/tag/v0.10.0

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.

This change is Reviewable

@Br1ght0ne Br1ght0ne force-pushed the Br1ght0ne:imag-0.10.0 branch from af70ed7 to 0ddfbd1 Jan 9, 2020
@Br1ght0ne
Copy link
Contributor Author

@Br1ght0ne Br1ght0ne commented Jan 9, 2020

@GrahamcOfBorg build imag

@matthiasbeyer
Copy link
Contributor

@matthiasbeyer matthiasbeyer commented Jan 9, 2020

Cool! I didn't manage to do this, because I do not have a lockfile in the repository. Seems like your PR works though.

Awesome! Keep me posted!

@Br1ght0ne
Copy link
Contributor Author

@Br1ght0ne Br1ght0ne commented Jan 9, 2020

@matthiasbeyer Is it possible to check it in the upstream repository, by any chance?

@Br1ght0ne Br1ght0ne force-pushed the Br1ght0ne:imag-0.10.0 branch from 2ae67ac to f88df7e Jan 9, 2020
@matthiasbeyer
Copy link
Contributor

@matthiasbeyer matthiasbeyer commented Jan 9, 2020

Possible yes, but I won't.

I won't because imag is in 0.x.y-state, which basically means that it can be used, but should not be considered final/stable/whatever. Adding a Cargo.lock file to the repository would result in that file changed for every other commit, which is just too much noise for me.
I will think about adding a Cargo.lock file to the repo for the 1.0.0 release, but not before that.

We could argue, though, that adding a Cargo.lock file for the release branches (so v0.10.x for the latest release right now), could be an option.

@Br1ght0ne
Copy link
Contributor Author

@Br1ght0ne Br1ght0ne commented Jan 9, 2020

@matthiasbeyer Sure, reasonable.


About the failing tests: do you know why? I'm more inclined to fix them than to just skip them.

@matthiasbeyer
Copy link
Contributor

@matthiasbeyer matthiasbeyer commented Jan 9, 2020

About the failing tests: do you know why? I'm more inclined to fix them than to just skip them.

The code is here: https://git.imag-pim.org/imag/tree/bin/core/imag-link/src/lib.rs?h=v0.10.x#n525

It could be that the ofborg instance does not allow access to the filesystem in tests. That could be the problem, because the tests in /bin all require filesystem-access. That's wrong actually. I am a bit too tired right now to make such bold guesses 😄

Copy link
Member

@yegortimoshenko yegortimoshenko left a comment

As you know, tests are currently failing, and as a result, derivation doesn't build:

running 6 tests
test tests::test_link_modificates ... ok
test tests::test_linking_links ... ok
test tests::test_linking_links_unlinking_removes_links ... FAILED
test tests::test_multilinking ... ok
test tests::test_linking_more_than_two ... ok
test tests::test_linking_and_unlinking_more_than_two ... FAILED

failures:

---- tests::test_linking_links_unlinking_removes_links stdout ----
thread 'tests::test_linking_links_unlinking_removes_links' panicked at 'assertion failed: `(left == right)`
  left: `Array([String("test2")])`,
 right: `Array([])`', bin/core/imag-link/src/lib.rs:520:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

---- tests::test_linking_and_unlinking_more_than_two stdout ----
thread 'tests::test_linking_and_unlinking_more_than_two' panicked at 'assertion failed: `(left == right)`
  left: `Array([String("test2"), String("test3")])`,
 right: `Array([])`', bin/core/imag-link/src/lib.rs:560:9


failures:
    tests::test_linking_and_unlinking_more_than_two
    tests::test_linking_links_unlinking_removes_links

If those tests are supposed to fail, consider skipping them.

@@ -19560,6 +19560,10 @@ in

iksemel = callPackage ../development/libraries/iksemel { };

imag = callPackage ../applications/office/imag {

This comment has been minimized.

@yegortimoshenko

yegortimoshenko Feb 21, 2020
Member

Nitpick: most CLI todo and contact list managers I know of are in applications/misc. Maybe this does belong to applications/office better, I'm not sure.

This comment has been minimized.

@matthiasbeyer

matthiasbeyer Feb 21, 2020
Contributor

I'm not sure either. It would certainly fit both, because its scope is PIM in general... I tend to prefer applications/misc though, as a "diary" for example is certainly not "office" 😄

@@ -0,0 +1,37 @@
{ stdenv
, lib

This comment has been minimized.

@yegortimoshenko

yegortimoshenko Feb 21, 2020
Member

Nitpick: stdenv.lib is also available :)

};

buildInputs = [ openssl pkg-config ]
++ lib.optional stdenv.isDarwin Security;

This comment has been minimized.

@yegortimoshenko

yegortimoshenko Feb 21, 2020
Member

Suggested change
++ lib.optional stdenv.isDarwin Security;
++ lib.optional stdenv.isDarwin Security;
sha256 = "1rkl4n3x2dk9x7705zsqjlxh92w7k6jkc27zqf18pwfl3fzz8f8p";
};

buildInputs = [ openssl pkg-config ]

This comment has been minimized.

@yegortimoshenko

yegortimoshenko Feb 21, 2020
Member

pkg-config is a build-time dependency:

-  buildInputs = [ openssl pkg-config ]
+  nativeBuildInputs = [ pkg-config ];
+  buildInputs = [ openssl ]

https://nixos.org/nixpkgs/manual/#ssec-stdenv-dependencies


LIBCLANG_PATH = "${llvmPackages.libclang}/lib";

cargoPatches = [ ./cargo-lock.patch ];

This comment has been minimized.

@yegortimoshenko

yegortimoshenko Feb 21, 2020
Member

@matthiasbeyer I understand the noise concern, and yet, consider:

If you’re building an end product, which are executable like command-line tool or an application, or a system library with crate-type of staticlib or cdylib, check Cargo.lock into git.

Currently, because lock file is not checked in, it's impossible to replicate the exact set of dependencies that you're developing imag with.

This comment has been minimized.

@matthiasbeyer

matthiasbeyer Feb 21, 2020
Contributor

You mean me, not Matthew. Yes, I consider checking it in for the next release in the release branch (not in the master branch though).

A patch release is pending (not sure though when I want to release it), I could add the Cargo.lock to the patchset release!

This comment has been minimized.

@yegortimoshenko

yegortimoshenko Feb 21, 2020
Member

So sorry for the typo!

@matthiasbeyer
Copy link
Contributor

@matthiasbeyer matthiasbeyer commented Feb 21, 2020

FWIW, tests do not fail on imag CI, so this seems to be nix-related.

@Br1ght0ne Br1ght0ne force-pushed the Br1ght0ne:imag-0.10.0 branch from f88df7e to dfadb68 Feb 26, 2020
@Br1ght0ne Br1ght0ne requested a review from yegortimoshenko Feb 26, 2020
@Br1ght0ne
Copy link
Contributor Author

@Br1ght0ne Br1ght0ne commented Feb 26, 2020

Thanks a lot @yegortimoshenko!

@Br1ght0ne Br1ght0ne force-pushed the Br1ght0ne:imag-0.10.0 branch from dfadb68 to 15b784f Feb 26, 2020

checkPhase = ''
cargo test -- \
--skip test_linking

This comment has been minimized.

@matthiasbeyer

matthiasbeyer Feb 27, 2020
Contributor

Would be nice to add a comment here, why this is skipped.

As said, in my CI this succeeds, so one might be curious why it is ignored here.

@matthiasbeyer
Copy link
Contributor

@matthiasbeyer matthiasbeyer commented Feb 29, 2020

Feel free to fast-forward merge matthiasbeyer@7532418

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented May 19, 2020

The CI fails with a sha256 error.

@puzzlewolf
Copy link
Contributor

@puzzlewolf puzzlewolf commented May 24, 2020

OfBorg-Log:

   Vendoring xdg-basedir v1.0.0 (/build/source/cargo-home.es6/registry/src/github.com-1ecc6299db9ec823/xdg-basedir-1.0.0) to imag-0.10.0-vendor.tar.gz/xdg-basedir
   Vendoring xml-rs v0.8.0 (/build/source/cargo-home.es6/registry/src/github.com-1ecc6299db9ec823/xml-rs-0.8.0) to imag-0.10.0-vendor.tar.gz/xml-rs
To use vendored sources, add this to your .cargo/config for this project:

installing
hash mismatch in fixed-output derivation '/nix/store/la60sfqsig6amjj59xvl47s8mdk3qm2q-imag-0.10.0-vendor.tar.gz':
  wanted: sha256:11l38wx5hi8a6gk4biqr11i05b0aa3mjphmzh6b8np9ghn0iym58
  got:    sha256:1ja1m3igjmmdbsjilmgfb3czlzlvpc6s4z4r5gsns2m807cd4kzq
cannot build derivation '/nix/store/sggs3sjxabyirwixw59kd6j1nymc3li5-imag-0.10.0.drv': 1 dependencies couldn't be built
error: build of '/nix/store/sggs3sjxabyirwixw59kd6j1nymc3li5-imag-0.10.0.drv' failed

I suspect this is related to the missing Cargo.lock. During the nix build, cargo pulls different versions of the dependencies than the ones used when the given cargoSha256 was computed, resulting in a different hash for the vendored dependencies.

@minijackson
Copy link
Contributor

@minijackson minijackson commented May 26, 2020

So I was trying my hand on packaging imag 0.10.1, which simplifies things since the Cargo.lock is distributed in the release (Thank you!), and after some debugging, I think I know why a lot tests are failing (more than just the linking ones from my experience).

Because the build is started from the nix daemon, it does not have a controlling tty, and it is a design choice in imag tools to treat stdin as a list of arguments if any, else use the provided command line arguments. Because imag uses the atty library to check if there is something in stdin, this leads to problems in basically every test that uses the command lines (i.e. ui tests).

For example, the test_grepping_not_available_string does basically this as setup:

imag-init --path $TMP_RTP
imag-create --rtp $TMP_RTP something

but imag-create sees that stdin is not a tty, and assumes it will be given the list of ids to create in stdin, and ignores something.

Possible solutions off the top of my head:

  • Change the behavior upstream to either:
    • Check that reading from stdin gives an empty string
    • Use both stdin and the command line as arguments
  • Disable every UI tests, and other tests that uses the CLI
  • Somehow create a (fake?) controlling TTY in the check phase (did a quick search in nixpkgs, and didn't find anything)

Here is my derivation file for reference:
https://gist.github.com/minijackson/096430eb6a4d0f70da12571a62d500fc

N.B.: Thank god strace exists in this world.

@matthiasbeyer
Copy link
Contributor

@matthiasbeyer matthiasbeyer commented May 26, 2020

From a packaging standpoint I'd say that disabling the ui tests are the best solution here. These are meant to test consistency across imag tools in their UI. If failing UI tests are included in a release, I as a developer did something wrong, not the packaging people.

So I guess disabling them would be the least pain in nixpkgs and also the most sensible thing to do.

@minijackson
Copy link
Contributor

@minijackson minijackson commented May 26, 2020

Well, I guess I went through that pain anyway :D

I found a why to create a fake TTY thanks to this SO answer.

Adding utillinuxMinimal to the checkInputs and replacing the last test command to script -qfec "cargo test --workspace" makes all tests pass (linking included)! \o/

Now that we have that, I'm not sure we should disable tests, IMHO. I think that if an update with failing ui tests were to happen, we should deal with it then, but I'm definitely up for discussions about that.

@matthiasbeyer
Copy link
Contributor

@matthiasbeyer matthiasbeyer commented May 26, 2020

Everything Working is of course always better!

Props to you for get it to work! 👍

@minijackson
Copy link
Contributor

@minijackson minijackson commented May 26, 2020

@filalex77 do you mind me opening another PR with imag 0.10.1? With the test fix, and the installation of completion scripts.

@Br1ght0ne
Copy link
Contributor Author

@Br1ght0ne Br1ght0ne commented May 26, 2020

@minijackson Of course, please go ahead! I would appreciate it if you add me as a maintainer (filalex77) too. Thanks for your help!

@Br1ght0ne Br1ght0ne closed this May 26, 2020
@minijackson minijackson mentioned this pull request May 27, 2020
4 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.