Modernize build system, README, and add Github Actions CI/CD #50
Conversation
|
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. |
Thanks!
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
👍 if there is consensus I'll set it to |
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 |
I also prefer to have the better defaults Since we can default to "best option available", I can set both to be
|
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)? |
|
Force pushed to change the defaults and tweak the README accordingly. Now
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
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: 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. |
|
By the way, about the
So it seems we can get rid of that feature and Seems like glibc has supported this for a very long time: |
|
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:
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. |
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. |
The reasoning for that is outlined in the OP:
Actually, I just noticed there is also a snap package and even a |
|
^ No-change rebase on top of recent changes to master. |
@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? |
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 #50 (comment). 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 Thank you for your understanding. |
- Builds for the following platforms (all amd64): - Ubuntu 18.04 - Ubuntu 20.04 - Windows (MSYS2/MinGW64) - macOS 10.15 - Builds for each platform with: - PTHREADS ON/OFF - OPENSSL ON/OFF - Each build produces a downloadable artifact that bundles: - The built mktorrent executable - The build-time generated compile_commands.json - The build-time generated graphviz link dependencies graph
Defining _FILE_OFFSET_BITS=64 is enough to properly support large files, for both 32-bit and 64-bit systems. See open(2) and feature_test_macros(7).
|
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 8ab1da7. 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. |
…ariables that are not meant to be expanded into lists
…and add macos-11 to build matrix
|
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-guiif()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)clangdthanks to CMake's-DCMAKE_EXPORT_COMPILE_COMMANDS=ONthat generates a compilation database.--graphvizflag (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=ONand-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
NEWpolicies. 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
masterbranch, 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:mktorrentbinary 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).graphvizgraph of the link dependenciescompile_commands.jsoncompilation 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_FILESfeature was completely removed in favor of a simpler, more universal solution. See Modernize build system, README, and add Github Actions CI/CD #50 (comment).