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

Make ironclad modular #44

Open
zen-wq opened this issue Jun 7, 2021 · 21 comments
Open

Make ironclad modular #44

zen-wq opened this issue Jun 7, 2021 · 21 comments

Comments

@zen-wq
Copy link

zen-wq commented Jun 7, 2021

I know it's a somewhat daunting task but the absolute majority of Ironclad uses I've seen only need MD5, SHA1, and byte-array-to-hex-string.

Frankly, I have no idea how to go about it. Splitting it into ironclad.digest and ironclad.cypher would be a good start, but more than half of the digests are also dead weight.

@luismbo
Copy link
Member

luismbo commented Jun 7, 2021

This would be useful. Perhaps the ironclad system can continue loading everything by default, but having the possibility to load core functionality and specific algorithms would be nice since ironclad does take a while to compile.

@zen-wq
Copy link
Author

zen-wq commented Jun 7, 2021

Yes, that's how I did it with cl-charms. ironclad system should just load all the separate ironclad modules so existing users don't break.

@glv2
Copy link
Collaborator

glv2 commented Jun 17, 2021

Frankly, I have no idea how to go about it. Splitting it into ironclad.digest and ironclad.cypher would be a good start, but more than half of the digests are also dead weight.

The "modules" that take the most time to compile are ciphers and digests, the rest compiles pretty fast in comparison. And as some digests are based on ciphers, having ironclad.cipher and ironclad.digest packages would not reduce the compilation time a lot. For example, using just one digest would still require compiling all ciphers and all digests.

To have a really smaller compilation time, we would need to have one system/package per cipher and per digest. That will take some work.

@luismbo
Copy link
Member

luismbo commented Jun 17, 2021

@glv2 why is that? Couldn't multiple systems share the same package?

@zen-wq
Copy link
Author

zen-wq commented Jun 17, 2021

They can. That would be one system per cipher/digest.

@luismbo
Copy link
Member

luismbo commented Jun 18, 2021

Here's a lightly tested first try: luismbo@879ab77

@glv2
Copy link
Collaborator

glv2 commented Jun 18, 2021

Here's a lightly tested first try: luismbo@879ab77

I took a look at your patch, here are a few notes:

keystream is not a cipher, it's a function to jump anywhere in the stream of pseudo-random bytes generated by some ciphers (not all ciphers support this operation).

The dependency list of some of the systems will have to be modified:

  • xchacha depends on chacha
  • xsalsa20 depends on salsa
  • skein depends on threefish
  • tree-hash depends on tiger
  • there might be others...

Also, aead, kdf, macs, prng and public-key depend on digests and ciphers, so I think we'll need to organize the systems in the following way:

  • ironclad/core, with just the basic stuff (common, conditions, generic, util, etc...)
  • ironclad/cipher/xxx depending on ironclad/core and sometimes on ironclad/cipher/yyy
  • ironclad/digest/xxx depending on ironclad/core and sometimes on ironclad/cipher/yyy or ironclad/digest/zzz
  • ironclad depending on all the previous systems, and containing the other modules (aead, kdf, macs, etc...)

Maybe it could be useful to also have the systems ironclad/ciphers, ironclad/digests, ironclad/aead, ironclad/macs, etc...

@luismbo
Copy link
Member

luismbo commented Jun 19, 2021

Regarding that last suggestion, would ironclad/ciphers load all the the ciphers?

@glv2
Copy link
Collaborator

glv2 commented Jun 19, 2021

Yes, and ironclad/digests would load all the digests. These would in fact be almost empty system definitions with a long list of dependencies.

@luismbo
Copy link
Member

luismbo commented Jun 21, 2021

Here's another go, even more untested than the previous one: luismbo@781d638. It adds a simple way to define dependencies and I've included the ones you've mentioned, but haven't reviewed the compiler warnings or anything like that to see if more are missing.

Not sure about ironclad/public-key vs ironclad/public-keys (likewise for aead and kdf).

A potential issue about this split is that changing a file, e.g. updating a macro, within ironclad/core does not trigger the recompilation of ironclad/cipher/foo, IIRC. It'd be nice to tell ASDF to do such recompilations.

@glv2
Copy link
Collaborator

glv2 commented Sep 3, 2021

@luismbo I created a split-systems branch with some fixes on top of your patch.
When loading the global ironclad system, it looks like things work fine and the tests pass.
I made a few tests loading only some subsystems, and they worked. Now, let's tests this more thoroughly before it can be merged into the master branch...
Please report any issue or weird behavior than you can find.

@luismbo
Copy link
Member

luismbo commented Sep 3, 2021

@glv2 thanks for picking this up. In terms of testing, perhaps we could load each subsystem individually (in a fresh lisp, let's say SBCL) then run the tests? (It'd be convenient if we could get away with detecting undefined ciphers/digests/etc rather than explicitly spliting the tests, but perhaps that could fail to detect missing dependencies, so perhaps explicitly splitting the tests is the way to go.) Does this seem like an approach worth exploring?

@glv2
Copy link
Collaborator

glv2 commented Sep 7, 2021

I guess the best solution will be for each subsystem to have its tests, and the test target for the global ironclad system will run the tests of all the subsystems. This will require an overhaul of our test system.

Do you think we should make these subsystems available to users in a release soon (as experimental feature), or wait until the new test system is done?

@luismbo
Copy link
Member

luismbo commented Sep 9, 2021

Releasing this as an experimental feature sounds like a good idea!

@glv2
Copy link
Collaborator

glv2 commented Sep 12, 2021

Subsystems are available as experimental feature in Ironclad 0.56.

@fjl
Copy link
Contributor

fjl commented Nov 15, 2021

It kind of feels like ASDF's package-inferred-system would be ideal for ironclad. I've been using it for all my own projects for the last two years or so, and it really works. There are specific advantages with package-inferred-system over defining your own structure: it forces you to get your dependencies right, and also forces creation of a sane package structure (because source files correspond 1-to-1 with lisp packages).

Using package-inferred-system for ironclad would mean that users only needing sha256 (for example), could depend on ironclad/digests/sha256 and have only this algorithm and its dependencies loaded into their image.

@glv2
Copy link
Collaborator

glv2 commented Nov 15, 2021

With the subsystems in Ironclad 0.56, you can do (asdf:load-system "ironclad/digest/sha256") and it will only load what is necessary for SHA256.

I'm not really a fan of the mandatory "one file = one package" of package-inferred-system. There are cases when I prefer having a package split in several files, or several packages in a file.

@fjl
Copy link
Contributor

fjl commented Nov 17, 2021

Understood, just wanted to bring it up because I feel it's underused. Also, I looked at the modularization changes and thought, wow, that's a lot of boilerplate for defining the additional systems/packages.

BTW, you can always define additional packages containing subsets of exported symbols with package-inferred-system, if you want larger groupings of functionality. As you may know, the idea behind P-I-S is mostly that it will figure out which source file to load based on the packages listed in :USE or :IMPORT-FROM. You can even define more than one package in a single file, the file just needs to also define the canonical package it was loaded for.

@fjl
Copy link
Contributor

fjl commented Jan 16, 2022

Having tried the subsystem thing now, here are two observations about the current approach.

(1) Since all API is provided by a single package, not all exported symbols are actually defined when only some subsystems are loaded. This is kind of obvious, but also really goes against the 'usual way'.

(2) For users of package-inferred-system the subsystem approach is not easy to work with. Loading, say "ironclad/digests/sha256" does not lead to a package of the same name being defined, so it's not possible to depend on it using

(defpackage ... (:use #:ironclad/digests/sha256))

I can work around this in my application by defining a wrapper system in .asd that depends on selected ironclad bits, then create a package to re-export this functionality. It's doable but needs boilerplate code :(.

@luismbo
Copy link
Member

luismbo commented Jan 17, 2022

@fjl you could also (eval-when (...) (asdf:load-system ...) at the start of your file, maybe?

@fjl
Copy link
Contributor

fjl commented Jan 17, 2022

It's not a good workaround because ASDF doesn't like it when Lisp files do their own operating during load. ASDF is supposed to know the dependencies of every file so it can plan the build properly.

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

No branches or pull requests

4 participants