imag: init at 0.10.0 #77362

Br1ght0ne · 2020-01-09T10:28:17Z

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

This change is

Br1ght0ne · 2020-01-09T10:34:18Z

@GrahamcOfBorg build imag

matthiasbeyer · 2020-01-09T12:15:28Z

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 · 2020-01-09T13:18:06Z

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

matthiasbeyer · 2020-01-09T14:49:06Z

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 · 2020-01-09T15:22:41Z

@matthiasbeyer Sure, reasonable.

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

matthiasbeyer · 2020-01-09T16:22:41Z

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 😄

lukateras

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.

lukateras · 2020-02-21T13:17:11Z

pkgs/top-level/all-packages.nix

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

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

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


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.

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" 😄

lukateras · 2020-02-21T13:24:59Z

pkgs/applications/office/imag/default.nix

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


Nitpick: stdenv.lib is also available :)

lukateras · 2020-02-21T13:49:54Z

pkgs/applications/office/imag/default.nix

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


Suggested change

++ lib.optional stdenv.isDarwin Security;

++ lib.optional stdenv.isDarwin Security;

lukateras · 2020-02-21T13:51:20Z

pkgs/applications/office/imag/default.nix

+    sha256 = "1rkl4n3x2dk9x7705zsqjlxh92w7k6jkc27zqf18pwfl3fzz8f8p";
+  };
+
+  buildInputs = [ openssl pkg-config ]


pkg-config is a build-time dependency:

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

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

lukateras · 2020-02-21T13:52:09Z

pkgs/applications/office/imag/default.nix

+
+  LIBCLANG_PATH = "${llvmPackages.libclang}/lib";
+
+  cargoPatches = [ ./cargo-lock.patch ];


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

https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html

https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries

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.

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!

So sorry for the typo!

matthiasbeyer · 2020-02-21T21:09:02Z

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

Br1ght0ne · 2020-02-26T20:10:07Z

Thanks a lot @yegortimoshenko!

matthiasbeyer · 2020-02-27T08:19:13Z

pkgs/applications/misc/imag/default.nix

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


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 · 2020-02-29T20:53:04Z

Feel free to fast-forward merge matthiasbeyer@7532418

doronbehar · 2020-05-19T03:23:42Z

The CI fails with a sha256 error.

puzzlewolf · 2020-05-24T22:48:37Z

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 · 2020-05-26T15:10:12Z

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 · 2020-05-26T15:28:20Z

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 · 2020-05-26T15:39:40Z

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 · 2020-05-26T15:53:09Z

Everything Working is of course always better!

Props to you for get it to work! 👍

minijackson · 2020-05-26T19:25:51Z

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

Br1ght0ne · 2020-05-26T21:52:19Z

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

Br1ght0ne force-pushed the imag-0.10.0 branch from af70ed7 to 0ddfbd1 Compare January 9, 2020 10:34

ofborg bot added 8.has: package (new) 11.by: package-maintainer 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Jan 9, 2020

Br1ght0ne force-pushed the imag-0.10.0 branch from 2ae67ac to f88df7e Compare January 9, 2020 13:24

lukateras reviewed Feb 21, 2020

View reviewed changes

Br1ght0ne force-pushed the imag-0.10.0 branch from f88df7e to dfadb68 Compare February 26, 2020 20:09

Br1ght0ne requested a review from lukateras February 26, 2020 20:09

imag: init at 0.10.0

15b784f

Br1ght0ne force-pushed the imag-0.10.0 branch from dfadb68 to 15b784f Compare February 26, 2020 20:11

matthiasbeyer reviewed Feb 27, 2020

View reviewed changes

Br1ght0ne closed this May 26, 2020

minijackson mentioned this pull request May 27, 2020

imag: init at 0.10.1 #89016

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

imag: init at 0.10.0 #77362

imag: init at 0.10.0 #77362

Br1ght0ne commented Jan 9, 2020 •

edited

Br1ght0ne commented Jan 9, 2020

matthiasbeyer commented Jan 9, 2020

Br1ght0ne commented Jan 9, 2020

matthiasbeyer commented Jan 9, 2020

Br1ght0ne commented Jan 9, 2020

matthiasbeyer commented Jan 9, 2020 •

edited

lukateras left a comment •

edited

lukateras Feb 21, 2020

matthiasbeyer Feb 21, 2020

lukateras Feb 21, 2020

lukateras Feb 21, 2020

lukateras Feb 21, 2020 •

edited

lukateras Feb 21, 2020 •

edited

matthiasbeyer Feb 21, 2020

lukateras Feb 21, 2020

matthiasbeyer commented Feb 21, 2020

Br1ght0ne commented Feb 26, 2020

matthiasbeyer Feb 27, 2020

matthiasbeyer commented Feb 29, 2020

doronbehar commented May 19, 2020

puzzlewolf commented May 24, 2020

minijackson commented May 26, 2020

matthiasbeyer commented May 26, 2020

minijackson commented May 26, 2020

matthiasbeyer commented May 26, 2020

minijackson commented May 26, 2020

Br1ght0ne commented May 26, 2020

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

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

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

	++ lib.optional stdenv.isDarwin Security;
	++ lib.optional stdenv.isDarwin Security;


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

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

imag: init at 0.10.0 #77362

imag: init at 0.10.0 #77362

Conversation

Br1ght0ne commented Jan 9, 2020 • edited

Motivation for this change

Things done

Br1ght0ne commented Jan 9, 2020

matthiasbeyer commented Jan 9, 2020

Br1ght0ne commented Jan 9, 2020

matthiasbeyer commented Jan 9, 2020

Br1ght0ne commented Jan 9, 2020

matthiasbeyer commented Jan 9, 2020 • edited

lukateras left a comment • edited

Choose a reason for hiding this comment

lukateras Feb 21, 2020

Choose a reason for hiding this comment

matthiasbeyer Feb 21, 2020

Choose a reason for hiding this comment

lukateras Feb 21, 2020

Choose a reason for hiding this comment

lukateras Feb 21, 2020

Choose a reason for hiding this comment

lukateras Feb 21, 2020 • edited

Choose a reason for hiding this comment

lukateras Feb 21, 2020 • edited

Choose a reason for hiding this comment

matthiasbeyer Feb 21, 2020

Choose a reason for hiding this comment

lukateras Feb 21, 2020

Choose a reason for hiding this comment

matthiasbeyer commented Feb 21, 2020

Br1ght0ne commented Feb 26, 2020

matthiasbeyer Feb 27, 2020

Choose a reason for hiding this comment

matthiasbeyer commented Feb 29, 2020

doronbehar commented May 19, 2020

puzzlewolf commented May 24, 2020

minijackson commented May 26, 2020

matthiasbeyer commented May 26, 2020

minijackson commented May 26, 2020

matthiasbeyer commented May 26, 2020

minijackson commented May 26, 2020

Br1ght0ne commented May 26, 2020

Br1ght0ne commented Jan 9, 2020 •

edited

matthiasbeyer commented Jan 9, 2020 •

edited

lukateras left a comment •

edited

lukateras Feb 21, 2020 •

edited

lukateras Feb 21, 2020 •

edited