Build is broken

[viega]

Since there's no tagged version, every little change ends up getting consumed by a bunch of downstream stuff (It's a dependency of a dependency of one of 3 dependencies we have).

And about an hour ago, builds started breaking on our Linux systems (tho not OS X).

/root/.nimble/pkgs2/nimcrypto-0.6.0-26ba256b08582f18a4139345944b6694564d056d/nimcrypto/hash.nim(50, 14) Error: cannot generate code for: bits

@arnetheduck seems it was your commit.

Not a great way to manage a project. Too bad it's such a pain in the neck for us to avoid this in the future.

[arnetheduck]

Good morning! I'm glad that you've found the project useful in the past and now have taken the time to eloquently express your displeasure at a change that was made which, although it works well for a lot of our code, broke yours - what was I thinking, not checking the effect it would have on your baby in particular. So sorry.

I know it can be daunting, but let's start with the basics:

• which Nim version are you using?
• what does the code look like that instantiates the digest?
• have you checked whether it is a known Nim bug?

Not a great way to manage a project.

Indeed, you have a great point that your project has gaps in this area - thanks for highlighting this as being a part of the problem - we've found it helpful to use for example lock files or submodules allowing us to evade this and many other such issues - maybe they can be useful for you as well!

[viega]

No, we do not use the code, we were using something that imports something that imports something that imports nimcrypto for a function we do not use (the Keekak implementation). The downstream dependency had functions that would call Keccak, but just basic operation. Since the next-to-last dependency didn't even declare nimcrypto in a nimble file, we had no clue this was being imported until the build broke.

Since it was just a simple little HMAC library, I simply excised the dependency.

But the build of your file trivially fails across multiple Linux flavors on nim 1.6.12.

And since I'm unlikely to spend any more time on your library and don't want to file a separate ticket, as one of the co-authors of GCM, you're 100% doing it wrong. The standard requires decrypt to authenticate and FAIL if the tag doesn't validate. The way nimcrypto is doing it defeats the whole purpose of GCM; it's essentially counter mode.

[viega]

And while I am sorry I got snitty over this, we do pin all our very few dependencies, so finding out something downstream we didn't even know we were dependent on, and were not calling into in any way, doesn't even have the hygiene to have tagged versions of releases, a few weeks before we're doing a big OSS release got me a little cranky.

I'll assume you were having a bad day as well, no worries :)

[arnetheduck]

we do pin all our very few dependencies,

clearly not ;)

Since the next-to-last dependency didn't even declare nimcrypto in a nimble file, we had no clue this was being imported until the build broke. ... so finding out something downstream we didn't even know we were dependent on

I'm glad we got something out of it - ie the real pro:s put sniffers and the like when they move their tag to a different commit, so in all honesty, had this been a real dependency of your code, you just dodged a bullet thanks to a triviality that could easily be fixed by adding an explicit commit hash to your .nimble file (ie requires "nimcrypto@#hash", a good idea to be doing in general).

Tags are not sacred, and nimble is often too buggy to use with tags as you envision, as happens in our case - there is more to this story than you assume and we don't know how every person on the internet uses the library so if you need a particular style of version management to fit your workflow, you could describe it in a feature request and we can consider whether it'll work with our workflow as well.

Regarding builds, the commit passes CI and testing with several Nim version meaning it does work quite well - even across the thousands of lines of code where we actively use it (it's one of the core pieces of our applications, unlike your case) so either you're using a different version than we test with or you have some unusual code. Either way, we'll need more context to find the source of your problem.

It is likely you hit a Nim bug - I found two while making this change and the code is significantly more ugly than it has to be as a result - perhaps you found a third bug, but we then we're back to square one: there's no way to tell from your report.

To conclude, I'm happy to take a look at whatever code you're having problems with to see if another workaround can fix it, but the patch solves an actual issue that is dear to us, so without more information, your only option is to pin a version you're happy with (you can do that for transitive dependencies also, more or less, if you manage to dodge the nimble bugs).

[viega]

Well, when you don't even know a downstream library is using something... And, as I said, I have removed the dependency.

But continuous releases without an extensive, required test suite before allowing merges into main is absolutely egregious, far worse than me not realizing we were somehow dependent on the library. You might have some test suite for whatever you're developing that uses this, but I see no such controls in place for this library.

Far more egregious still is the seeming lack of understanding in crypto that permeates the library, the broken GCM implementation being a good example.

And I triggered the bug independently just by trying to import a module containing only this code:

import nimcrypto

type
  Keccak512Digest* = array[0..63, char]

proc hash_keccak512*(s: string): Keccak512Digest = cast[Keccak512Digest](keccak512.digest(s))
proc hmac_keccak512*(key, data: string): Keccak512Digest =
  result = cast[Keccak512Digest](sha3_512.hmac(key, data))

[arnetheduck]

Confirmed / known compiler bug in 1.6.12 - your Nim is out of date and since you don't know/control which dependencies you're using, this will keep happening for you.

We'll consider the options, but if you want to expedite a fix, PR:s are welcome.

[viega]

You don't seem to understand, even though I said it multiple times, I ripped out the dependency (after I posted but, before you ever responded), and have only been following the thread to try to help the library improve.

Being a library, you shouldn't expect people to be constantly upgrading... 1.6.12 is only about 6 weeks old. If you're serious about this library being decent, maybe either more thorough automated testing, or, this is one of the reasons why few libraries do continuous releases.

Though I have to say, if you're serious about this library in any way, then you really need to design it so that people can't easily shoot themselves in the foot. I'd be happy to give more advice elsewhere if you cared to make it not be bad, but considering the conversation, I feel like you don't care, and that's understandable!

All the best with it.

[cheatfate]

Fixed in #70.