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

Multiple error messages of type: println is not a member of fmt #549

Closed
stefanos82 opened this issue May 29, 2023 · 16 comments
Closed

Multiple error messages of type: println is not a member of fmt #549

stefanos82 opened this issue May 29, 2023 · 16 comments
Assignees
Labels
bug Something isn't working

Comments

@stefanos82
Copy link

I've just pulled the latest changes and attempted to compile the code:

Building btop++ (v1.2.13) Linux x86_64
Compiling src/btop_draw.cpp
Compiling src/btop.cpp
Compiling src/btop_config.cpp
Compiling src/btop_input.cpp
src/btop.cpp: In function ‘void argumentParser(const int&, char**)’:
src/btop.cpp:116:30: error: ‘println’ is not a member of ‘fmt’
  116 |                         fmt::println(
      |                              ^~~~~~~
src/btop.cpp:132:30: error: ‘println’ is not a member of ‘fmt’
  132 |                         fmt::println("btop version: {}", Global::Version);
      |                              ^~~~~~~
src/btop.cpp:148:38: error: ‘println’ is not a member of ‘fmt’
  148 |                                 fmt::println("ERROR: Preset option needs an argument.");
      |                                      ^~~~~~~
src/btop.cpp:155:38: error: ‘println’ is not a member of ‘fmt’
  155 |                                 fmt::println("ERROR: Preset option only accepts an integer value between 0-9.");
      |                                      ^~~~~~~
src/btop.cpp:164:30: error: ‘println’ is not a member of ‘fmt’
  164 |                         fmt::println(" Unknown argument: {}\n"
      |                              ^~~~~~~
src/btop.cpp: In function ‘void clean_quit(int)’:
src/btop.cpp:269:22: error: ‘println’ is not a member of ‘fmt’
  269 |                 fmt::println(std::cerr, "{}ERROR: {}{}{}", Global::fg_red, Global::fg_white, Global::exit_error_msg, Fx::reset);
      |                      ^~~~~~~
src/btop.cpp: In function ‘int main(int, char**)’:
src/btop.cpp:763:22: error: ‘println’ is not a member of ‘fmt’
  763 |                 fmt::println("WARNING: Could not get path user HOME folder.\n"
      |                      ^~~~~~~
src/btop.cpp:768:30: error: ‘println’ is not a member of ‘fmt’
  768 |                         fmt::println("WARNING: Could not create or access btop config directory. Logging and config saving disabled.\n"
      |                              ^~~~~~~
make: *** [Makefile:276: obj/btop.o] Error 1
make: *** Waiting for unfinished jobs....
20%  -> obj/btop_config.o             (1.5MiB) (05s)
20%  -> obj/btop_input.o              (1.4MiB) (05s)
30%  -> obj/btop_draw.o               (2.2MiB) (06s)
@stefanos82 stefanos82 added the bug Something isn't working label May 29, 2023
@stefanos82
Copy link
Author

stefanos82 commented May 29, 2023

The addition of fmt in lib directory for existing cloned repos cause confusion to building process.

I had to re-clone it and repeat the whole building process to force it to use the forked version located in lib/fmt/ than the old version I have installed on my system.

We need to let users know they need to either re-clone the whole repo or init the submodule and pull the latest changes.

Update: I personally just tried git submodule update --init --recursive inside the old repo btop/lib/fmt/ and worked just fine; it initialized it and then pulled the necessary HEAD detouched at ... and built just fine.

@aristocratos can we update the README to include the necessary command(s) that users need to use to pull changes from submodules too?

For transmission, we use the following commands for updating the repo and then build the project with the latest changes:

git submodule foreach --recursive git clean -xfd
git pull --rebase --prune
git submodule update --recursive

@stefanos82
Copy link
Author

For some reason, GitHub does not let me fork btop 🤔 or edit the README file...anyway.

Can we add a sub-step called 2a. Updating Repository and write below it

$ cd btop
$ git pull --rebase --prune
$ git submodule update --recursive

with a tip:

In case you haven't pulled any updates for a long time, please run `git pull --rebase --prune` 
and make sure there is a new folder in your project's root directory called `lib`; if indeed there 
is such folder, run `git submodule update --init --recursive` and pull the necessary submodule repo 
we need to build our application.

@doj
Copy link

doj commented Jun 4, 2023

I have the same issue with compiling btop on my Gentoo Linux. It uses g}} 12.2.1 and compiles with c++20. I have just recloned the btop repository, and I see the lib/fmt/ directory. But that directory is empty.

@stefanos82
Copy link
Author

Weird...go inside that directory and run git submodule update --init --recursive; it should pull the latest changes.

@doj
Copy link

doj commented Jun 5, 2023

hmm, it does, but you have to be in the lib/ directory. It does nothing if I'm in the toplevel directory, or in the lib/fmt/ directory. And I should have paid more attention to the updated README.md file. I didn't use "--recursive" with the git clone command. The compilation now works for me with the current HEAD revision.

@aristocratos
Copy link
Owner

@stefanos82 @doj

Can we add a sub-step called 2a. Updating Repository and write below it

Gonna add some more documentation. Probably gonna be a lot of these issues in the coming weeks for people who compile themself and just run git pull or miss the note in the readme if pulling fresh.
Whish you could edit the hint github gives in the "Code" button or for --recursive to be the default...

@doj
Copy link

doj commented Jun 6, 2023

the best would be, if make could detect that this file fails to build and print a message to check the lib/fmt/ directory.
Or maybe the file could check for something if #ifdef and then output on #error message?

@NexAdn
Copy link
Contributor

NexAdn commented Jun 21, 2023

I've just noticed that the build works if libfmt 10 is installed on the system (Gentoo Linux in my case). Does this mean we now have a hard dependency on libfmt 10? Because there are problems for older projects which are incompatible with libfmt 10, hence requiring older libfmt to stick around on operating systems for a little longer.

If it's not crucial for btop's operation, I'd rather not have a hard dependency on libfmt 10, but make the code at least compatible to libfmt 9, which is the version available in Debian bookworm (and hence the version one might expect on many server systems where btop might be used as a resource monitor). This would mean that println must not be used, though.

@aristocratos
Copy link
Owner

@NexAdn
Did you read the previous messages in this thread? The project is using libfmt from a git submodule.
And that's the version it's dependent on. But you will always have the correct version with the commands described above.

Unless you got exactly the same version as in the submodule installed on your system, it's not guaranteed to work.

NexAdn added a commit to NexAdn/btop that referenced this issue Jun 21, 2023
Simply replacing calls to fmt::println with calls to fmt::print allows
btop to be compiled with libfmt 8 and 9, allowing compatibility with the
system libraries of more operating systems.

Issue: aristocratos#549
@NexAdn
Copy link
Contributor

NexAdn commented Jun 21, 2023

Well, for local development that might be nice, but for packaging I'm gonna download a source tarball which won't include the submodule. Usually, submodules don't bring much of a benefit compared to pulling in dependencies as system packages, but can increase the maintenance overhead for system packagers. Thus, unless it is crucial not to use a system package, I'd rather avoid using submodules when packaging software for a system.

Compatibility with system libfmt 8, 9 and 10 can be established by simply replacing calls to fmt::println with calls to fmt::print. But to be honest, the places where fmt::println was used were so simple that they could've easily be replaced by a few std::cout, std::cerr and std::clog lines without causing much harm to readability.

Since I now have the patch ready anyway, I'm gonna open PR. If you're willing to allow system libfmt to be used for building btop, please consider merging it. I'd be very grateful to have the option not to use git submodules.

@aristocratos
Copy link
Owner

@NexAdn
I think a better option in that case would be to reverse #537
Only a few header files are needed since we are using the header only version.

I rather not that libfmt functionality is limited by what is available in version 8 and restrict future functionality.

@aristocratos
Copy link
Owner

aristocratos commented Jun 21, 2023

Actually this might be a better idea: AcademySoftwareFoundation/MaterialX#317 (comment)

Including a btop-complete.tar.gz with every release that contains all source + submodule source.

@NexAdn
Copy link
Contributor

NexAdn commented Jun 21, 2023

I rather not that libfmt functionality is limited by what is available in version 8 and restrict future functionality.

Well, the current limit is only that println is no longer allowed. But since this can be considered as a convenience wrapper around print, I don't see how banning println in the code limits the functionality in any way.

Including a btop-complete.tar.gz with every release that contains all source + submodule source.

I'm still not a fan of bundling dependencies in the first place, but keeping the submodules for development and yet still have only a single tarball with all deps in it to download for packaging is a good compromise if you'd like to stick with bundled dependencies.

@aristocratos
Copy link
Owner

aristocratos commented Jun 21, 2023

@NexAdn

Well, the current limit is only that println is no longer allowed. But since this can be considered as a convenience wrapper around print, I don't see how banning println in the code limits the functionality in any way.

If you look at https://github.com/fmtlib/fmt/releases you'll see that there's a lot more than just println changed between version 8 and 10.
fmtlib have just begun being implemented and having it be constrained by a version that is 2 years old now just to be more convenient for packaging doesn't really sit well with me.

I'm still not a fan of bundling dependencies in the first place

Why not? All bundled dependencies are "header only", no extra build steps and you'll always have the "correct" version.

@NexAdn
Copy link
Contributor

NexAdn commented Jun 24, 2023

there's a lot more than just println changed between version 8 and 10.

Of course there is. However, since btop works perfectly fine with 8 through ten when println is replaced with print, it seems like println is the only feature from v10 that is actually used in btop. So adding compatibility with 8 and 9 is imho really not that big of a deal. If you'd like to use other v10 features which aren't available in 8 and 9 and have to “good” alternatives there, requiring v10 is of course a sensible decision.

Why not?

Not particularly relevant in this case, but I have dealt with packages where devs didn't care about getting their bundled dependencies clean, leading to headers or libs of bundled dependencies being installed in the system (which of course then cause conflicts with the system-installed dependencies). Simply moving bundled libs to another directory can then break the package itself (if not done correctly), while keeping the bundled libs in place might cause breakage to other packages on the system which now use that bundled lib instead of the real library from the system package. Fixing this can be very annoying.

Apart from that, with bundled libs a system package maintainer has no control over per-package compiler/linker flags or patches required to make the package work on a system or to apply critical security fixes. Luckily, this is only rarely needed, but when the library is bundled other packages, it's hard to track all the packages that need patching of the bundled library down. With a system package, you always have one single package that needs patching, not dozens.

Admittedly, these problems are not that relevant in the case of btop, but they're a general motivation for me to keep bundled libs out of software (or at least optional) as much as possible. Most of the time, keeping a dependency's version pinned isn't really required to make the software work and having compatibility across a reasonable range of dependency versions isn't that much of a hassle.

@aristocratos
Copy link
Owner

If you'd like to use other v10 features which aren't available in 8 and 9 and have to “good” alternatives there, requiring v10 is of course a sensible decision.

The most crucial thing is having the UTF-8 support working correctly when I start to convert all the big strings in btop_draw.cpp so the widths of the strings can be correctly calculated. And there seems to be a lot of fixes to UTF-8 between version 8 and 10.
So while it might compile fine with version 8, there might be instances of text not displaying correctly/spilling in to the wrong "pane" and so on with the older versions.

Not particularly relevant in this case, but I have dealt with packages where devs didn't care about getting their bundled dependencies clean [...]

I can understand that.
One of the reasons there are so few dependencies and those that are included are all "header only" is to make the compilation as easy and self contained as possible while also keeping it easy to statically link the binary. The only real dependency should be the compiler and it's included libraries.
Everything is compiled in to one binary and if for example statically compiled with musl (like the packages in the releases), it will run on any Linux system with kernel 2.6.39 or newer.

I've even thought about adding a version checker and updater in the static releases since the binary will keep on working regardless of any updates/changes to the system it's running on.

Software releases on Linux through distribution packaging is kind of a mess in general, in regards to the different standards, rules, formats and dependency version availability between distros. And alternatives like snap, flatpack and appimages aren't really that good either with the risk of issues with performance, access to the running system, file sizes and so on.

In a perfect world "in regards to btop++ :)", everyone would use the static releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants