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

C++17 std::fs #20460

Closed
maflcko opened this issue Nov 23, 2020 · 13 comments · Fixed by #20744
Closed

C++17 std::fs #20460

maflcko opened this issue Nov 23, 2020 · 13 comments · Fixed by #20744

Comments

@maflcko
Copy link
Member

maflcko commented Nov 23, 2020

C++17 std::fs would be useful for a stronger rename method (#20435) and dropping boost fs in favor of the stdlib (#20744).

However, we currently use bionic to compile gitian binaries, which uses gcc-7 by default. If we are ok switching to gcc-8 on bionic for our gitian builds and everyone compiling from source on bionic, we could start using std::fs

@vasild
Copy link
Contributor

vasild commented Nov 23, 2020

If this would require just one more package to the apt-get install ... line which users already execute, then I don't see why not go forward with this.

apt-get install foo bar ... gcc-8
export CC=gcc8 CXX=g++8
... compile as usual ...

@kiminuo
Copy link
Contributor

kiminuo commented Nov 23, 2020

@vasild I believe, you need to add -lstdc++fs for gcc-8 to make it work.

It seems it is important where the flag is placed (at least on my Ubuntu 18.04 & gcc-8):

// This is simple "main.cpp" file to test compiling of a program with std::filesystem on gcc-8.

// Ubuntu 18.04
// works:         g++-8 --std=c++17 main.cpp -lstdc++fs 
// does not work: g++-8 --std=c++17 -lstdc++fs main.cpp 

#include <string>
#include <iostream>
#include <filesystem>
namespace fs = std::filesystem;

int main()
{
    std::string path = "C:\\cpp_filesystem";
    for (auto & p : fs::directory_iterator(path))
        std::cout << p << std::endl;
}

It would be great if somebody could verify my claim though.

Some resources:

@promag
Copy link
Member

promag commented Nov 23, 2020

I've verified @kiminuo claim:

docker run --rm -it ubuntu:bionic
apt update && apt install gcc-8 g++-8
# build above example

Edit: @kiminuo however it shouldn't matter here, as we compile and link separately.

@promag
Copy link
Member

promag commented Nov 23, 2020

Concept ACK.

@kiminuo
Copy link
Contributor

kiminuo commented Nov 23, 2020

@promag I was unlucky to go through link phase when doing:

./autogen.sh && ./configure CC=gcc-8 CXX="g++-8" LDFLAGS="-lstdc++fs" --with-incompatible-bdb && make -j10

on kiminuo:feature/2020-06-replace-boost-filesystem-with-cpp17-filesystem git branch (#19245).

It would be great help resolving this.

Note: It has helped me to add manually in the generated makefile -lstdc++fs as follows:

bitcoin-cli$(EXEEXT): $(bitcoin_cli_OBJECTS) $(bitcoin_cli_DEPENDENCIES) $(EXTRA_bitcoin_cli_DEPENDENCIES) 
	@rm -f bitcoin-cli$(EXEEXT)
	$(AM_V_CXXLD)$(bitcoin_cli_LINK) $(bitcoin_cli_OBJECTS) $(bitcoin_cli_LDADD) $(LIBS) -lstdc++fs

@promag
Copy link
Member

promag commented Nov 23, 2020

@kiminuo oh it also happens just linking.

@practicalswift
Copy link
Contributor

Concept ACK

@vasild
Copy link
Contributor

vasild commented Nov 24, 2020

Concept ACK

I gather gcc 9 does not require -lstdc++fs. Is it as available as gcc 8 on the relevant platforms?

@kiminuo
Copy link
Contributor

kiminuo commented Dec 15, 2020

@vasild

I gather gcc 9 does not require -lstdc++fs. Is it as available as gcc 8 on the relevant platforms?

gcc-9 is not supported on Ubuntu 18.04 as far as I know. Ubuntu 20.04 (focal) has gcc-9 package (https://packages.ubuntu.com/search?suite=focal&arch=any&searchon=names&keywords=gcc-9). So answer is probably no.

@fanquake
Copy link
Member

I will put together some changes for this 📁 .

@fanquake fanquake linked a pull request Feb 26, 2021 that will close this issue
@jonatack
Copy link
Member

jonatack commented Mar 2, 2021

Concept ACK

@maflcko maflcko added this to the 23.0 milestone Mar 3, 2021
@maflcko
Copy link
Member Author

maflcko commented Mar 3, 2021

We've had reports of people unable to compile because of outdated compilers that can't handle the C++17 in our master branch. Once the 22.0 release is out, we might get more complaints about that. I think bumping the compiler versions even higher for the 22.0 release might be a bit too aggressive and rushed.

Assigned 23.0 for now, so that we can collect some feedback after the 22.0 release.

@maflcko
Copy link
Member Author

maflcko commented Oct 21, 2021

Any further discussion can be done in the pull

@maflcko maflcko closed this as completed Oct 21, 2021
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants
@vasild @fanquake @jonatack @promag @maflcko @practicalswift @kiminuo and others