-
Notifications
You must be signed in to change notification settings - Fork 194
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
First sketch for an easier configuration #301
Conversation
Some comments/a wishlist:
|
I don't understand the point of such a thing ... that's exactly why tommath_superclass.h exists
|
The only thing I could see is having a parser which is going through "your own sources"/the sources using libtommath and then spits out something that can be put in tommath_superclass.h |
For me the current mechanism is not sufficient at least. Then there are still some deficiencies like def file generation, issue with bn_conversion, bn_deprecated, no support for amalgam. I don't think the class mechanism is particularly nice but ymmv. What czurnieden proposes is simpler. The downside however is that it needs a script. |
what exactly is missing?
those are other issues, but yes they must be solved... btw.
the class mechanism supports that
I don't see a difference in adding lines to a file called Maybe I also don't get the advantage of the proposed approach!? Please feel free to explain me what's different and better :-) |
Yes. But I would like to have it such that only used functions are included.
Sure, we could also stay with a file full of defines. I think here we are exploring what kind of alternative approach we could take. This will not necessarily end up in ltm. But if it turns out to be technically better, why not? As I said from my pov, I don't think the class/superclass mechanism is perfect. However the important issues we are observering are due to the insufficient parsing in helper.pl. Besides the one-file-per-function issue of bn_conversion and bn_deprecated there is also another one: Basically mp_mul has a "weak dependency" on s_mp_mul_karatsuba. This should be reflected via the configuration such that mp_mul does not automatically require karatsuba. This could be parsed more easily given #262 since we could look for MP_HAS. So instead of conflating two different issues here we could also split the discussion:
|
Fine by me, should we merge #300 then and work on an improved version in another PR? or should we use #300 to already implement an improved version (not necessarily by @nijtmans) As I didn't get an answer to my question "what is missing?" I have to guess from your comment... so you don't want to always build all files!? and only have the used files in the amalgamated version?! |
I suggest to merge it and we improve it afterwards. It is sufficient as a first step.
Yes. Not always building all files wasn't my idea, this is what @czurnieden came up with here. But for the amalgam, this is what I would like. And I need the tommath_class.h to be non buggy since it still includes too much, i.e., the weak dependencies. |
(BIt late for me to chime in, hu?)
The main advantage of the The script combines (sort of) The first has no real advantage over the old system, the default for LTM would still be to deliver a complete set in
The file Doing it by manipulating the makefiles directly would make it possible to get rid of all the preprocessor stuff that tried to replace what autoconf/cmake does anywhere else. Some branchings in LTM's code rely on these directives and would need to get changed. They are to be found in the files A lot of work? Yes. Advantage of getting rid of the old system: using cmake to generate all the makefiles including MSVC project files. It is easy to use cmake to exclude/include files from/to the build and it can be automated. Cmake because it is more portable than autoconf and not everyone hates it. It also has a GUI. The use of Cmake would also make this PR obsolete ;-) Until than: this PR can be extended to include a (G)UI for ticking off boxes (including the small one-line explanations in the individual files as help) which I personally wouldn't find very useful but, as you Problems are with
You can only do so much in a q&d hack in the middle of the night ;-) That would be the work for an option like |
I don't know what you mean by weak dependencies? the specific implementations? e.g. TM-mul, Kara-mul etc. for mul?!? so you want a whitelist of functions?
@czurnieden said Cmake... I say ccmake (for the ncurses UI) and I'm not sure what @minad will say... hooray.. or nay |
Please no cmake and please no autotools.
mp_mul optionally takes advantage of s_mp_mul_karatsuba but it does not necessarily depend on it since there is a preprocessor ifdef. This I call a "weak dependency" (or optional dependency). In contrast to that, mp_add necessarily depends on s_mp_add. This is a (strong) dependency. In tommath_class.h the BN_MP_MUL_C block defines BN_S_MP_MUL_KARATSUBA_C since it has no concept of a weak dependency. |
What I want: If I enable |
okay, if you think you can solve this problem, go on! [Edit] ...and you really think it's worth the effort...[/Edit] as long as you didn't solve it, simply add this to
|
Given #262 and MP_HAS it is pretty easy since this allows to heuristically detect optional dependencies. And it could be integrated easily with the current infrastructure. |
|
||
$content =~ s{/\*.*?\*/}{}gs; | ||
my $list = ""; | ||
foreach my $line (split /\n/, $content) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this already work together with bn_conversion bn_deprecated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bn_deprecated
? It's deprecated, don't care.
bn_conversion
? Not checked yet, but unlikely. If it gets splitted into single files it will work.
It also points out a disadvantage: this script relies on LTM's naming scheme for the source files instead of their content.
The naming scheme is needed because the content of some files get generated by a preprocessor macro. It's down to only one file now, bn_conversion
. Please run gcc -E
and C&P the generated code at the end into correctly named individual files. That would help a lot.
No, wait, there is also bn_cutoffs.c
, which needs to be treated separately. But a single file with a handful of constants is acceptable.
s_mp_toom_mul => 1, | ||
s_mp_toom_sqr => 1 | ||
); | ||
my @dependency_list = (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is for weak dependencies I assume?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Note to self: be a bit more verbose in commenting the code)
Yes.
But it is hardcoded and you can only switch all on/off for now.
It also ignores the extra-small alternatives to some functions as listed in the example for RSA in tommath_superclass.h
{ | ||
my ($path, @entries) = @_; | ||
my $tcpath; | ||
my $content = "/* LibTomMath, multiple-precision integer library -- Tom St Denis */\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you are just writing a flat list right? I would suggest we remove tommath_class and tommath_superclass.h and replace it with tommath_config.h just containing the flat list. This would be preferable to having multiple different mechanisms around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you are just writing a flat list right?
Yes?
I would suggest we remove tommath_class and tommath_superclass.h and replace it with tommath_config.h just containing the flat list.
There might be users who already have some kind of automatisms installed. I would like to avoid such brutality just for the sake of prettiness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be users who already have some kind of automatisms installed.
IMO that's the way we should go. Why not create a fresh tommath_superclass.h
from this tool?
open my $fh, "<", $f or die "FATAL: read_rawfile() cannot open file '$f': $!"; | ||
binmode $fh; | ||
return do { local $/; <$fh> }; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File::Slurper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To quote the aptly named Cenk K. Uygur of TyT fame: "Have at it, Hoss!" ;-)
This file contains a lot of C&P from helper.pl
which has been glued together with nothing more but vague memories from bygone times, so don't expect too much, thank you.
print $fh $data or die "FATAL: write_file() cannot write to '$f': $!"; | ||
close $fh or die "FATAL: write_file() cannot close '$f': $!"; | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File::Slurper...come on, it should be used because of the name already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we are at it: this one is harmless but others are not. You might look up all pronounceable names at urbandictionary.com first. (Example: slong
for signed long
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a regime change here at Github, forgot?
This includes a now significant chance that somebody (e.g.: one of the shareholders) complains and another somebody (e.g.: an intern at MS) runs a badly written script over the sources here.
I don't think that it will get as bad as the rotten mess that currently happens over at YT but I also don't want to learn the differences between github and e.g.: gitlab.
But serious: no unintended(!) giggles, please, thank you.
my $change_makefiles = 0; | ||
|
||
GetOptions( "s|source-dir=s" => \$source_dir, | ||
"t|tommath-dir=s" => \$tommath_dir, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we avoid the over configuration? Just always run in ./
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just always run in ./
That's the default and it was cheap to offer options.
You can also install it and use if for different versions of LTM. Not useful now but once a new version comes out where all the deprecated stuff is actually gone you might need to keep older versions for legacy reasons.
And don't ignore the legacy part, there are people out there who make a nice penny from supporting old software and even bought an old laptop from ebay to get a fully functional parport to be able to read the backups stored with zipdrives ;-)
The goal of this script has changed a little bit. It is now a freestanding tool to adapt LTM to the user's needs only, like @sjaeckel suggested. It is also a bit more "Unix-y" this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this as a freestanding tool?
To part the tools for the LTM developers from the tools for the users of LTM. The latter is gathered in etc
for now and the first one in helper.pl
.
It is not the best way to do it but the organisation in LTM is a bit odd. The documentation is in doc
, the logs are in logs
, in mtest
is the stuff necessary for that test and a copy of Michael Fromberger's mpi.c
, in demo
is the testing stuff but no demo, and etc
is quite a mixed bag. All in all a real mess. The organization scheme common in many larger source-trees src
, build
, docs
, scripts
, and tests
with the configuration stuff in the root directory is not that bad and can be used for LTM, too.
But that's a lot of work with no gain but that satisfied feeling once it's done and until than…
Did @sjaeckel order this?
I'm no-one's servant nor any-one's master; will neither give nor take orders.
Unless you pay me well, of course, such that I can pay a professional to look over my english grammar and point out all the ambiguous parts in my texts, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this as a freestanding tool?
I'm seeing this as the prototype-phase w/o interfering with helper.pl too much (less merge conflicts and stuff) so it's fine like that
Did @sjaeckel order this? ;)
lol
no but I would've started like that as well I guess ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@czurnieden Just to clarify, this was a joke. Sorry if you got offended. Or probably I got caught by you joking back...
@sjaeckel I agree fully to have it freestanding for the prototyping phase. But above, @czurnieden said
The goal of this script has changed a little bit. It is now a freestanding tool to adapt LTM to the user's needs only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, this was a joke.
You put a smiley at the end, so…?
Sorry if you got offended
That's nigh impossible, that bar hangs very high, it needs a lot to make this friendly bear remembering that a bear is omnivore.
And also: as somebody who dishes out with both hands I can hardly complain if I have to take some, too ;-)
Or probably I got caught by you joking back...
Yeah, I'm too complicated sometimes. The play with the increasingly limping metre in "I'm no-one's servant…" was probably a bit over the top?
But back to biz:
@sjaeckel I agree fully to have it freestanding for the prototyping phase. But above, @czurnieden said "The goal of this script has changed a little bit. It is now a freestanding tool to adapt LTM to the user's needs only"
Yes, we have different points of view. I don't see it as a bad thing. especially if the outcome is the same ;-)
I named the functions I copied from helper.pl
or gen.pl
differently if I changed them and kept their names otherwise, as you would expect. With the exception of some clashes in the short options it should be no problem to put them all in helper.pl
in about half an hour, including the essential coffee break. It would be a much bigger effort the other way around.
Or to put it more bluntly: insisting on putting it all into helper.pl
now comes too early.
} | ||
else { | ||
write_header($td, @dependency_list); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if there is a single --update commandline, and then both makefiles and tommath_config.h header are rewritten.
Furthermore I would love to have an amalgam being generated too, but I could also contribute this at some later point 😀 It would replace gen.pl
EDIT: Since you still have the depmap, maybe you could also import the callgraph logic from helper.pl? While we don't commit the callgraph anymore in git (which is good), I like it to debug the dependency scanning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if there is a single --update commandline, and then both makefiles and tommath_config.h header are rewritten.
You need only one to make it work, but is is of course no problem to remove this condition and allow both options simultaneously.
Furthermore I would love to have an amalgam being generated too
Give them an inch and they'll take an ell. ;-)
I'd rather change gen.pl
to read from tommath_class.h
which has either everything listed between LTM_ALL
and endif
or contains the reduced set generated by this script. It would also be more "Unix-y".
That would ignore any manually made changes in tommath_superclass.h
, of course, but the current gen.pl
ignores them, too.
Since you still have the depmap, maybe you could also import the callgraph logic from helper.pl?
In one comment you ask: "Could we avoid the over configuration?" and here you want something extra? ;-)
The callgraph logic from helper.pl
s is already used in this script, didn't want to reinvent the wheel.
Yes, the question is if I should base it on the current config infrastructure or on the script @czurnieden is working on. It already looks quite good what he is doing here I think. However I am not sure if it already handles bn_deprecated and bn_conversion correctly. In the meantime the PRs #292, #300 and #304 are ready from my pov if you don't want the backlog to grow too long. EDIT: Also #303 if you agree with removing the separate function. |
e1b0470
to
681925a
Compare
278f50f
to
961b277
Compare
Note to self: be careful with |
The script The file I do not plan to add more features but I might have forgotten some, so feel free to shout at me for doing so. All of the functions in The splitting of Some folks here want to weave these scripts into And now let me see how many bugs my product actually has. *sigh* |
…_class.h if generated by etc/make_small_lib.pl
Regarding having this as a separate tool: I think it would be better to have this as part of helper.pl since we are doing all the generation tasks there right now. Similar to calltree.txt, tge statified amalgam could be .gitignored and generated always for helper.pl --update-files. I like what you are doing here generally. I would just like it a bit simpler with less config and less moving parts. Only config file + helper.pl - gen.pl. |
Just an idea What do you think about having a tool which can be dropped in like $CC and writes out a list of potential ltm-functioncalls? So I can take my project and do something like
and probably @fperrad or @karel-m who have AFAICT the biggest Perl skills here have something to add as well (probably some ideas, comments or even some sneaky implementation ;) ) |
You mentioned this idea of a scanner for external projects before. For me it would be of limited usefulness in contrast to what @czurnieden proposes here. I am using such a minimal ltm subset, but right now I simply select the needed functions by hand. Given the config system proposed here, my configuration process that would be even simpler. But usually I only have to do this once. An external scanner wouldn't be used often, since the scanner would only touch the config (tommath_superclass.h or the config file introduced in this PR). However I would suggest we discuss the external scanner in a separate issue, since it is independent of this PR. It can be made such that it works with the current config system and it could be made such that it works with the new system proposed by czurnieden here. |
(Scary! And in bold even! ;-) )
It's currently implemented in
The default is to write a new It has the option to manipulate the makefiles to scrape another couple of bytes off by not producing "empty shells" (yes, I am aware of e.g.: This script does not make backups of the manipulated files yet, it relies on being run in an active git repository and its ability to do a
There is no distinct configuration file needed anymore,
Functionality of
It's already build in, it was a by-product.
That would be highly appreciated! |
Close, see #160 for the reason |
A rough(!) sketch for a (half)automated configuration. Just list the things you want in
make_config.conf
and runmake_config.pl
to get the correct makefiles.It still needs a
make clean
before themake
orar
still gathers everything.I tried to reuse as much as possible from
helper.pl
because such thing should be put there, but started with an isolated script to ease your judgment.A
make new_file
orhelper.pl --update-makefiles
should return the original makefiles.Updating MSVC project files is a todo, I don't know enough to do it safely.