Open FranciscoPombal opened 4 years ago
This seems very promising. I like it very much. Hopefully it gets merged. A sidenote: I think OpenSSL and pthreads could be enabled by default (if they are found), and even long options could be enabled in my opinion.
@uno20001
This seems very promising. I like it very much. Hopefully it gets merged.
Thanks!
A sidenote: I think OpenSSL and pthreads could be enabled by default (if they are found),
Personally, I don't like enabling/disabling features based on whether they are found or not (i.e., "enabling by side-effect"). However, I don't mind setting the defaults of either or both of these features to ON
. If the user's system doesn't support them, CMake will fail with a helpful message anyway.
and even long options could be enabled in my opinion.
:+1: if there is consensus I'll set it to ON
by default.
Personally, I don't like enabling/disabling features based on whether they are found or not
That's understandable, and I don't think many end users will build this piece of software themselves, but I like if something Just Works™, and selects the best option available. For mktorrent
using openssl
and pthreads
provides substantial benefits in terms of performance, that's why I suggest they be enabled if they are found.
@uno20001
That's understandable, and I don't think many end users will build this piece of software themselves, but I like if something Just Works™, and selects the best option available. For
mktorrent
usingopenssl
andpthreads
provides substantial benefits in terms of performance, that's why I suggest they be enabled if they are found.
I also prefer to have the better defaults ON
, but I like to give the user full control of the rest. This PR just started off with everything OFF
by default because it seemed like for this project, defaulting to the "most compatible option available" was preferable to the "best option available".
Since we can default to "best option available", I can set both to be ON
by default, and tweak the script's UX to be something like:
ON
.OPENSSL_ROOT_DIR
] or pass -DMKTORRENT_OPENSSL/PTHREADS=OFF
to disable building with this feature"-DMKTORRENT_OPENSSL=OFF -DMKTORRENT_PTHREADS=OFF
I like to give the user full control of the rest.
Certainly, I very much agree. I didn't intend to imply that the choice should be taken away from the user. By the way, is it possible - I assume it is - to have CMake automatically detect OpenSSL/pthreads and use them, but if they are not available, fall back to not using them (unless explicitly specified otherwise by the user, of course)?
@uno20001
Force pushed to change the defaults and tweak the README accordingly. Now MKTORRENT_LONG_OPTIONS
, MKTORRENT_OPENSSL
and MKTORRENT_PTHREADS
are all on by default.
Certainly, I very much agree. I didn't intend to imply that the choice should be taken away from the user.
It's not about choice, it's about control and explicitness. IMO, the script should use the "best options" by default, but if that's not possible, it should simply inform the user that's not possible, and defer to the user any further decisions about what options to use. I don't want the script deciding project-level options automatically. It's to easy to miss a message saying "package XXX detected -> feature X enable" and easily leads to bug reports due to reproducibility issues and confusion because of that[1]. It is preferable to "fail build due to feature FOO not available" -> user re-runs with -DFOO=OFF
.
By the way, is it possible - I assume it is - to have CMake automatically detect OpenSSL/pthreads and use them, but if they are not available, fall back to not using them (unless explicitly specified otherwise by the user, of course)?
Yes, it is possible to auto detect packages and use them if available, and I'm aware many people like/use this style. The logic would be something like:
#...
find_package(OpenSSL ${requiredOpenSSLVersion})
if(OpenSSL_FOUND)
#....
but like I said above, this is what I want to avoid. If it's part of the defaults and the build fails because of it, I'd rather require the user be explicit about overriding defaults.
[1]: For example, suppose user Bob builds mktorrent on their machine, and CMake can't find Pthreads. If CMake enables/disables stuff automatically, Bob may not notice that mktorrent builds without Pthreads. Then, Bob runs mktorrent and notices the poor performance. They may figure out what's happening on their own, or they may open a bug report, wasting their time and contributors' time, until someone asks for the output of the configure step and notices that Pthreads were not used, explains the situation to Bob and closes the issue as "Not an issue".
You could argue "Well, we could ask users to paste the configure output in the issue template". But if, instead, you make the option control explicit, you only need to ask users to paste one line - the CMake configure command-line they used. With fully explicit option control, you always know the project-level feature set that's being attempted to be enabled just from looking the configure command-line, no matter the machine.
@uno20001
By the way, about the MKTORRENT_LARGE_FILES
feature. I just read https://man7.org/linux/man-pages/man7/feature_test_macros.7.html and https://man7.org/linux/man-pages/man2/open.2.html. Here are the interesting bits:
feature_test_macros(7)
:
_LARGEFILE64_SOURCE
(...)
New programs
should not employ this macro; instead _FILE_OFFSET_BITS=64
should be employed.
_LARGEFILE_SOURCE
This macro was historically used to expose certain functions (...)
New programs should not employ this macro; defining
_XOPEN_SOURCE as just described or defining _FILE_OFFSET_BITS
with the value 64 is the preferred mechanism to achieve the
same result.
_FILE_OFFSET_BITS
Defining this macro with the value 64 automatically converts
references to 32-bit functions and data types related to file
I/O and filesystem operations into references to their 64-bit
counterparts. This is useful for performing I/O on large
files (> 2 Gigabytes) on 32-bit systems. (Defining this macro
permits correctly written programs to use large files with
only a recompilation being required.)
64-bit systems naturally permit file sizes greater than 2
Gigabytes, and on those systems this macro has no effect.
open(2)
:
O_LARGEFILE
(LFS) Allow files whose sizes cannot be represented in an
off_t (but can be represented in an off64_t) to be opened.
The _LARGEFILE64_SOURCE macro must be defined (before
including any header files) in order to obtain this
definition. Setting the _FILE_OFFSET_BITS feature test macro
to 64 (rather than using O_LARGEFILE) is the preferred method
of accessing large files on 32-bit systems (see
feature_test_macros(7)).
So it seems we can get rid of that feature and O_LARGEFILE
in calls to open()
and always define _FILE_OFFSET_BITS=64
. What do you think?
Seems like glibc has supported this for a very long time: https://en.wikipedia.org/wiki/Large-file_support https://users.suse.com/~aj/linux_lfs.html
I read those parts sometime in the past, and I agree. They could be gotten rid of.
@uno20001
I read those parts sometime in the past, and I agree. They could be gotten rid of.
The latest commit implements these changes.
If you are adamant about having the build script enable/disable features automatically based on auto-detection of packages or other things, then I would propose including the following patch, or something similar:
diff --git a/src/main.c b/src/main.c
index a3158d1..6370adc 100644
--- a/src/main.c
+++ b/src/main.c
@@ -132,7 +132,30 @@ int main(int argc, char *argv[])
};
/* print who we are */
- printf("mktorrent " VERSION " (c) 2007, 2009 Emil Renner Berthing\n\n");
+ printf("mktorrent " VERSION " (c) 2007, 2009 Emil Renner Berthing\n\n"
+ "Built with the following features:\n\n"
+#ifdef USE_LONG_OPTIONS
+ "MKTORRENT_LONG_OPTIONS ON\n"
+#else
+ "MKTORRENT_LONG_OPTIONS OFF\n"
+#endif
+#ifdef NO_HASH_CHECK
+ "MKTORRENT_NO_HASH_CHECK ON\n"
+#else
+ "MKTORRENT_NO_HASH_CHECK OFF\n"
+#endif
+#ifdef USE_OPENSSL
+ "MKTORRENT_OPENSSL ON\n"
+#else
+ "MKTORRENT_OPENSSL OFF\n"
+#endif
+#ifdef USE_PTHREADS
+ "MKTORRENT_PTHREADS ON\n"
+#else
+ "MKTORRENT_PTHREADS OFF\n"
+#endif
+ "MKTORRENT_MAX_OPENFD " "%d" "\n"
+ "MKTORRENT_PROGRESS_PERIOD " "%d" "\n\n", MAX_OPENFD, PROGRESS_PERIOD);
/* process options */
init(&m, argc, argv);
Sample outputs:
mktorrent -h
mktorrent 1.1-861176~dirty (branch: master) (c) 2007, 2009 Emil Renner Berthing
Built with the following features:
MKTORRENT_LONG_OPTIONS ON
MKTORRENT_NO_HASH_CHECK OFF
MKTORRENT_OPENSSL ON
MKTORRENT_PTHREADS ON
MKTORRENT_MAX_OPENFD 100
MKTORRENT_PROGRESS_PERIOD 200000
Usage: mktorrent [OPTIONS] <target directory or filename>
Options:
-a, --announce=<url>[,<url>]* : specify the full announce URLs
additional -a adds backup trackers
-c, --comment=<comment> : add a comment to the metainfo
-d, --no-date : don't write the creation date
-h, --help : show this help screen
-l, --piece-length=<n> : set the piece length to 2^n bytes,
default is 18, that is 2^18 = 256kb
-n, --name=<name> : set the name of the torrent
default is the basename of the target
-o, --output=<filename> : set the path and filename of the created file
default is <name>.torrent
-p, --private : set the private flag
-s, --source=<source> : add source string embedded in infohash
-t, --threads=<n> : use <n> threads for calculating hashes
default is the number of CPU cores
-v, --verbose : be verbose
-w, --web-seed=<url>[,<url>]* : add web seed URLs
additional -w adds more URLs
Please send bug reports, patches, feature requests, praise and
general gossip about the program to: mktorrent@rudde.org
mktorrent -v
mktorrent 1.1-861176~dirty (branch: master) (c) 2007, 2009 Emil Renner Berthing
Built with the following features:
MKTORRENT_LONG_OPTIONS ON
MKTORRENT_NO_HASH_CHECK OFF
MKTORRENT_OPENSSL ON
MKTORRENT_PTHREADS ON
MKTORRENT_MAX_OPENFD 100
MKTORRENT_PROGRESS_PERIOD 200000
fatal error: must specify the contents, use -h for help
This way, figuring out the important build options with which a certain binary was built is never a problem, even after the fact.
But, IMO, that should be handled in another PR, there are enough changes in this one.
If you are adamant about having the build script enable/disable features automatically based on auto-detection of packages or other things [...]
Oh, don't get me wrong, I am not adamant about it. Your reasoning certainly makes sense, and I would be perfectly fine if it stayed as it is now. Furthermore, I am not the one in charge, so it's not up to me. I just tried to provide some feedback and voice my preferences.
Is cmake version 3.17 absolutely necessary? That version is quite new (~released only 4 months ago) and many package repositories may only have older versions of cmake available.
@jackm
Is cmake version 3.17 absolutely necessary? That version is quite new (\~released only 4 months ago) and many package repositories may only have older versions of cmake available.
The reasoning for that is outlined in the OP:
Why CMake 3.17 minimum requirement? Isn't that too new?
Although this script probably works fine with older versions, I did not care to find the minimum possible version that this script works well with. With CMake one should use the latest possible version, to always benefit from the latest improvements, bug fixes, and
NEW
policies. For a lot of programs, this is impractical for many users, because their distributions often lag behind. But in the case of CMake, it is easy to keep up with the latest version since Kitware provides up-to-date official binaries and even an APT repository for all major platforms. MSYS2 is also very quick to provide the latest versions for use with MinGW. And, if it's really needed, compiling CMake from source is not that bad/hard :).
Actually, I just noticed there is also a snap package and even a pip
binary package.
^ No-change rebase on top of recent changes to master.
Is cmake version 3.17 absolutely necessary?
Although this script probably works fine with older versions [...]
@jackm so probably not. If you have an older version, feel free to check if it works, and report back.
@Rudde what do you think?
Was there a change in the ownership of the repo? Should I bother fixing the merge conflicts, or is there no interest in this change?
Was there a change in the ownership of the repo? Should I bother fixing the merge conflicts, or is there no interest in this change?
Yes, there was. I would definitely like to see this pull request being merged, however, I have started doing major changes some time ago - which I would like to finish before the end of the year (or as my time permits) -, thus I figure it's better to put off fixing the conflicts and merging this PR until after that.
@pobrn Any plans to merge this soon? I fixed all the conflicts and added on extra commit implementing the idea mentioned in https://github.com/pobrn/mktorrent/pull/50#issuecomment-661939155. I would be interested in perhaps implementing support for V2 torrents after this.
@FranciscoPombal and anyone who's following the development here, I apologize for staying silent. Contrary to my initial plans, I was not able to finish what I planned by the end of last year.
But the good news is that now I think I'm more or less finished with what I planned. There are still a couple issues I want to iron out, but I'm pretty certain the lion's share of what I wanted to do is done - this includes BitTorrent v2 metafile generation.
I would prefer if you could wait until I push the first public version of what will become mktorrent
2.0 and then adapt this PR to that - only if you still want to do it, of course - since it'll significantly differ from the current version.
Thank you for your understanding.
@pobrn
Thanks for the prompt response.
I just did some fore-pushes to fix some CI problems. They are fixed now. You can see the results here: https://github.com/FranciscoPombal/mktorrent/runs/2355014222
Thanks to the CI, a build failure was detected on MinGW. The fatal error was a regression introduced by https://github.com/pobrn/mktorrent/commit/8ab1da736b89416fe8a1146f685a570f48dd5b01. This problem could have been avoided (or at least documented) had this PR been merged sooner. Perhaps the authors of that change could have been urged to fix the build failure with some conditional code before the change was merged. As such, and to prevent such problems in the future, I would recommend merging this one before the overhaul.
For these reasons, I think you have more to gain if you rebase your work in progress on top of these changes instead of the other way around. Let me know what you think.
@pobrn
I made some minor cleanups. Any new ETA to share?
Hi, here's a couple of patches mainly centered around modernizing the the project's build system and providing CI/CD via GitHub Actions.
Build system
Basically, This PR gets rid of the Makefiles in favor of (modern) CMake. Besides CMake itself, there is no other new mandatory build-time dependency.
This change nets:
mktorrent
-specific options are "namespaced" underMKTORRENT_.*
, so they appear grouped undercmake-gui
if()
salad) using purelytarget_*
commands and generator expressions, which makes it easy to support cross-platform projects (for example, with a couple of changes to the code base itself, it would be possible to compile for Windows with MSVC with little or no changes to current build script)make
(e.g.ninja
)clangd
thanks to CMake's-DCMAKE_EXPORT_COMPILE_COMMANDS=ON
that generates a compilation database.--graphviz
flag (not that relevant for a small project with a maximum of 2 direct link dependencies, but still) Here's an example of the generated graph on Ubuntu 18.04 (amd64) with-DMKTORRENT_PTHREADS=ON
and-DDMKTORRENT_OPENSSL=ON
:Why CMake 3.17 minimum requirement? Isn't that too new?
Although this script probably works fine with older versions, I did not care to find the minimum possible version that this script works well with. With CMake one should use the latest possible version, to always benefit from the latest improvements, bug fixes, and
NEW
policies. For a lot of programs, this is impractical for many users, because their distributions often lag behind. But in the case of CMake, it is easy to keep up with the latest version since Kitware provides up-to-date official binaries and even an APT repository for all major platforms. MSYS2 is also very quick to provide the latest versions for use with MinGW. And, if it's really needed, compiling CMake from source is not that bad/hard :).GitHub Actions
This PR provides a simple CI/CDs setup with GitHub Actions (which is free for public projects and their forks). The change to CMake sinergizes quite well with this workflow.
The workflow provides CI for every PR or commit to the
master
branch, for different combinations of platform and build options (Ubuntu 18.04/20.04, Windows, macOS, OpenSSL/pthreads ON/OFF). Each build produces a downloadable artifact bundle (hence the "CD" part) consisting of:mktorrent
binary for each platform, with the git revision baked into the version string (when OpenSSL is used, it is statically linked on both Windows and macOS).graphviz
graph of the link dependenciescompile_commands.json
compilation databaseHere's an example run in my fork: https://github.com/FranciscoPombal/mktorrent/actions/runs/174852635
In the future, this workflow can be extended or serve as inspiration for a workflow that provides automatic official releases.
README and other changes
src/
to clean up the project's structure.Notes
Let me know if, for legacy reasons, any of these apply, and I'll adjust the PR accordingly:
src/
is not desirable for some reason.EDITS - additional changes
USE_LARGE_FILES
feature was completely removed in favor of a simpler, more universal solution. See https://github.com/Rudde/mktorrent/pull/50#issuecomment-661761151.