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

Try to fix some issues with slowness in the MacOS build. #4280

Merged
merged 4 commits into from
Dec 12, 2023
Merged

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Dec 8, 2023

The MacOS CI build was fairly out-of-date, which caused slowness.

Previously dependencies took 42 minutes:
https://github.com/p4lang/p4c/actions/runs/7143318180/job/19454526195?pr=4278

Now they are taking ~1-2 minutes:
https://github.com/p4lang/p4c/actions/runs/7144321547/job/19457683307?pr=4280

Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any obvious problems here, but since I have no experience with macOS and brew I don't think I should approve this (i.e. I don't know why brew link is needed for boost).

Can we safely build on macOS 12, or can there still be users on 11? (I'm not sure how OS versions and updates work for macOS :-D).

Copy link
Contributor

@jnfoster jnfoster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting an error about std::binary_function in json.h. It seems this is now deprecated.

/Users/nate/shared/p4c/lib/ordered_map.h:51:32: error: no template named 'binary_function' in namespace 'std'; did you mean '__binary_function'?
    class value_compare : std::binary_function<value_type, value_type, bool> {
                          ~~~~~^~~~~~~~~~~~~~~
                               __binary_function
/Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/__functional/binary_function.h:49:1: note: '__binary_function' declared here
using __binary_function = __binary_function_keep_layout_base<_Arg1, _Arg2, _Result>;
^
In file included from /Users/nate/shared/p4c/lib/json.cpp:17:
In file included from /Users/nate/shared/p4c/lib/json.h:31:
/Users/nate/shared/p4c/lib/ordered_map.h:65:26: error: no template named 'binary_function' in namespace 'std'; did you mean '__binary_function'?
    struct mapcmp : std::binary_function<const K *, const K *, bool> {
                    ~~~~~^~~~~~~~~~~~~~~
                         __binary_function
/Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/__functional/binary_function.h:49:1: note: '__binary_function' declared here
using __binary_function = __binary_function_keep_layout_base<_Arg1, _Arg2, _Result>;
^
2 errors generated.

I'm on an M1 Macbook running OS X 14 (Sonoma), with this C++ compiler.

The CXX compiler identification is AppleClang 15.0.0.15000040

@jnfoster jnfoster self-requested a review December 11, 2023 12:07
@fruffy
Copy link
Collaborator Author

fruffy commented Dec 11, 2023

Getting an error about std::binary_function in json.h. It seems this is now deprecated.

This is apparently a deprecated function which has been removed in C++17. The common advice is to just remove it. A little strange that the other compilers do not complain.

#4283

@vlstill
Copy link
Contributor

vlstill commented Dec 11, 2023

This is apparently a deprecated function which has been removed in C++17. The common advice is to just remove it. A little strange that the other compilers do not complain.

Could be libstdc++ ABI would break if they remove it, it is still widely used in the standard library itself (as of GCC 11). Apple might care a bit less about ABI then the open-source world.

@jafingerhut
Copy link
Contributor

@fruffy I have never used the macOS build of any open source P4 dev tools before, despite using a Mac for personal use -- I've only ever compiled it in a Linux VM.

@fruffy
Copy link
Collaborator Author

fruffy commented Dec 11, 2023

Can we safely build on macOS 12, or can there still be users on 11? (I'm not sure how OS versions and updates work for macOS :-D).

Me neither, but Homebrew was warning about the usage of MacOS 11. considering it deprecated.

Adding @rst0git because I believe he used the MacOS build in the past.

Copy link
Member

@rst0git rst0git left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jnfoster
Copy link
Contributor

I still can't build on my Mac. Seems likely there are a bunch of other binary_function instances in the codebase...

For example: https://github.com/p4lang/p4c/blob/fruffy/macos_ci/lib/ordered_set.h#L56

@fruffy
Copy link
Collaborator Author

fruffy commented Dec 11, 2023

I still can't build on my Mac. Seems likely there are a bunch of other binary_function instances in the codebase...

For example: https://github.com/p4lang/p4c/blob/fruffy/macos_ci/lib/ordered_set.h#L56

Should have been more thorough... will try to fix this in this PR directly.

@jnfoster
Copy link
Contributor

Seems to be working now... will let you know when it completes.

@jnfoster
Copy link
Contributor

Worked for most backends.

Graphs had this error:

/opt/homebrew/include/boost/multi_index/detail/bucket_array.hpp:239:19: error: use of undeclared identifier 'bad_archive_exception'; did you mean 'multi_index::detail::bad_archive_exception'?
  throw_exception(bad_archive_exception());
                  ^
/opt/homebrew/include/boost/multi_index/detail/bad_archive_exception.hpp:25:8: note: 'multi_index::detail::bad_archive_exception' declared here
struct bad_archive_exception:std::runtime_error
       ^
In file included from /Users/nate/shared/p4c/backends/graphs/graphs.cpp:17:
In file included from /Users/nate/shared/p4c/backends/graphs/graphs.h:33:
In file included from /opt/homebrew/include/boost/graph/adjacency_list.hpp:36:
In file included from /opt/homebrew/include/boost/graph/named_graph.hpp:18:
In file included from /opt/homebrew/include/boost/multi_index/hashed_index.hpp:35:
/opt/homebrew/include/boost/multi_index/detail/index_node_base.hpp:120:19: error: use of undeclared identifier 'bad_archive_exception'; did you mean 'multi_index::detail::bad_archive_exception'?
  throw_exception(bad_archive_exception());
                  ^
/opt/homebrew/include/boost/multi_index/detail/bad_archive_exception.hpp:25:8: note: 'multi_index::detail::bad_archive_exception' declared here
struct bad_archive_exception:std::runtime_error
       ^
2 errors generated.
make[2]: *** [backends/graphs/CMakeFiles/p4cgraphs.dir/graphs.cpp.o] Error 1
make[1]: *** [backends/graphs/CMakeFiles/p4cgraphs.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

@fruffy
Copy link
Collaborator Author

fruffy commented Dec 11, 2023

Worked for most backends.

Graphs had this error:

/opt/homebrew/include/boost/multi_index/detail/bucket_array.hpp:239:19: error: use of undeclared identifier 'bad_archive_exception'; did you mean 'multi_index::detail::bad_archive_exception'?
  throw_exception(bad_archive_exception());
                  ^
/opt/homebrew/include/boost/multi_index/detail/bad_archive_exception.hpp:25:8: note: 'multi_index::detail::bad_archive_exception' declared here
struct bad_archive_exception:std::runtime_error
       ^
In file included from /Users/nate/shared/p4c/backends/graphs/graphs.cpp:17:
In file included from /Users/nate/shared/p4c/backends/graphs/graphs.h:33:
In file included from /opt/homebrew/include/boost/graph/adjacency_list.hpp:36:
In file included from /opt/homebrew/include/boost/graph/named_graph.hpp:18:
In file included from /opt/homebrew/include/boost/multi_index/hashed_index.hpp:35:
/opt/homebrew/include/boost/multi_index/detail/index_node_base.hpp:120:19: error: use of undeclared identifier 'bad_archive_exception'; did you mean 'multi_index::detail::bad_archive_exception'?
  throw_exception(bad_archive_exception());
                  ^
/opt/homebrew/include/boost/multi_index/detail/bad_archive_exception.hpp:25:8: note: 'multi_index::detail::bad_archive_exception' declared here
struct bad_archive_exception:std::runtime_error
       ^
2 errors generated.
make[2]: *** [backends/graphs/CMakeFiles/p4cgraphs.dir/graphs.cpp.o] Error 1
make[1]: *** [backends/graphs/CMakeFiles/p4cgraphs.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

This might be solved by #4147. The corresponding PR has not been reviewed yet.

@fruffy fruffy merged commit def92ac into main Dec 12, 2023
13 checks passed
@fruffy fruffy deleted the fruffy/macos_ci branch December 12, 2023 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants