Modernize build system, README, and add Github Actions CI/CD

[FranciscoPombal]

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:

• easy customization of the build with command line options - no more manually inspecting or editing Makefiles to figure out the build options
• mktorrent-specific options are "namespaced" under MKTORRENT_.*, so they appear grouped under cmake-gui
• maintainable declarative build logic (no if() salad) using purely target_* 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)
• easy integration with the rest of the CMake-aware projects ecosystem and dependency management
• good cross-platform support (CMake) for free
• support for multiple build tools for free besides make (e.g. ninja)
• better interoperability with language-intelligence tools such as LLVM clangd thanks to CMake's -DCMAKE_EXPORT_COMPILE_COMMANDS=ON that generates a compilation database.
• A nice visualization of link dependencies via the --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:

• the built 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).
• the generated graphviz graph of the link dependencies
• the generated compile_commands.json compilation database

Here'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

• The README is now more fleshed out and documents the usage of the build options and procedure quite thoroughly. It also has the CI/CD workflow badge. Check it out here: https://github.com/FranciscoPombal/mktorrent/blob/master/README.md
• Compilable sources were moved to src/ to clean up the project's structure.
• A couple minor changes were made to the source code to support longer version strings.

Notes

Let me know if, for legacy reasons, any of these apply, and I'll adjust the PR accordingly:

• it is not acceptable to get completely rid of the Makefile-based build system (after all, ultimately, they can coexist, but I'm against projects supporting more than one build system - it's an unnecessary maintenance burden, pollutes the project tree, and can lead to confusion).
• the minimum required CMake version really needs to be lower for some reason.
• moving the compilable sources to src/ is not desirable for some reason.

EDITS - additional changes

• the 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.

[uno20001]

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.

[FranciscoPombal]

@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.

[uno20001]

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.

[FranciscoPombal]

@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 using openssl and pthreads 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:

1. User attempts to build with all defaults; by default both OpenSSL and pthreads are ON.
2. If the user's system does not have OpenSSL or pthreads, configure step fails with some message like "OpenSSL/pthreads not found on your system, install the required packages and/or set the appropriate variables [like OPENSSL_ROOT_DIR] or pass -DMKTORRENT_OPENSSL/PTHREADS=OFF to disable building with this feature"
3. User decides they don't want the features so they re-run the configuration with -DMKTORRENT_OPENSSL=OFF -DMKTORRENT_PTHREADS=OFF
4. Build succeeds.

[uno20001]

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)?

[FranciscoPombal]

@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.

[FranciscoPombal]

@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

[uno20001]

I read those parts sometime in the past, and I agree. They could be gotten rid of.

[FranciscoPombal]

@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: [email protected]

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.

[uno20001]

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.

[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.

[FranciscoPombal]

@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.

[FranciscoPombal]

^ No-change rebase on top of recent changes to master.

[uno20001]

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.

[uno20001]

@Rudde what do you think?

[FranciscoPombal]

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?

[pobrn]

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.

[FranciscoPombal]

@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.

[pobrn]

@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.

[FranciscoPombal]

@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.

[FranciscoPombal]

@pobrn

I made some minor cleanups. Any new ETA to share?