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.

Sign up for GitHub

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

Jump to bottom

FreeBSD: Fix to build APPLEN #170

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

FreeBSD: Fix to build APPLEN #170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions source/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,10 +251,22 @@ target_include_directories(appleii PRIVATE
)

# this one appears in header files
if ("${CMAKE_SYSTEM_NAME}" STREQUAL "FreeBSD")
# FreeBSD could already have "zip.h" (from libzip) in /usr/local/include
# which takes precedence than the one in /usr/local/include/minizip.
# "zip.h" from libzip is not what we want here.
# The following changes to use include path /usr/local/include for minizip
# so that the code can include "minizip/zip.h" explicitly.
target_include_directories(appleii PUBLIC
${CMAKE_CURRENT_BINARY_DIR} # for config.h
${MINIZIP_INCLUDE_DIRS}/..
Copy link
Owner

Choose a reason for hiding this comment

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

I see the issue, but it is not limited to FreeBSD. What happens if I install https://packages.ubuntu.com/noble/libzip-dev ?
There must be a more general solution

Copy link
Owner

@audetto audetto Jun 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I have tried and there are no issues. The reason must be

  1. if I want minizip, then I add -I/usr/include/minizip and although zip.h is ambiguous, this explicit -I wins over the default one (used by libzip)
  2. If I want zip, no problem as I do not add the minizip -I
  3. I can't use them both.

So, the issue is probably related to a user who have both libzip and minizip in /usr/local which breaks the (weak) order of includes.

Copy link
Owner

@audetto audetto Jun 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

OK, now I see better. In FreeBSD they are all installed in /usr/local so they cannot coexist.
Which is really odd, maybe nobody is using it, someone should file a bug.

The fact is that the command pkg-config minizip -cflags in FreeBSD returns -I/usr/local/include/minizip.

Copy link
Author

Choose a reason for hiding this comment

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

I think it is exactly the order of -I problem. pkg-config is returning the right path for minizip. But some other libs is likely returning the common /usr/local/include and happens to be earlier in the include path. The kind of problem does happen to FreeBSD ports occasionally too. Ideally for FreeBSD /usr/local/include should always be the last path. But that's very difficult to control. Neither is to have /usr/local/include/minizip always in front, just not obvious to me how within CMake that can be done reliably. If all Linux distros always put minizip headers in its own subdirectory, the hack here could work universally and so in AppleWin code we could simply #include <minizip/zip.h>.

)
else()
target_include_directories(appleii PUBLIC
${CMAKE_CURRENT_BINARY_DIR} # for config.h
${MINIZIP_INCLUDE_DIRS}
)
endif()

target_link_libraries(appleii PRIVATE
${YAML_LIBRARIES}
Expand Down
4 changes: 4 additions & 0 deletions source/DiskImageHelper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,11 @@ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
#include "Common.h"

#include "zlib.h"
#ifdef __FreeBSD__
#include "minizip/unzip.h"
#else
#include "unzip.h"
#endif

#include "CPU.h"
#include "DiskImage.h"
Expand Down
4 changes: 4 additions & 0 deletions source/DiskImageHelper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@

#include "DiskDefs.h"
#include "DiskImage.h"
#ifdef __FreeBSD__
#include "minizip/zip.h"
#else
#include "zip.h"
#endif

#define GZ_SUFFIX ".gz"
#define GZ_SUFFIX_LEN (sizeof(GZ_SUFFIX)-1)
Expand Down
3 changes: 3 additions & 0 deletions source/Tfe/DNS.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
#include <arpa/inet.h>
#include <sys/socket.h>
#include <netdb.h>
#ifdef __FreeBSD__
#include <sys/socket.h> // AF_INET.
Copy link
Owner

Choose a reason for hiding this comment

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

no need to add ifdef, I will commit this.

Copy link
Author

@kiyolee kiyolee Jun 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Agree with that. This is just to explain the PR.
Should I update my PR? Or would you just fix your code in your way and drop the PR?
That's totally fine to me.

#endif
#endif

uint32_t getHostByName(const std::string & name)
Expand Down
3 changes: 3 additions & 0 deletions source/Uthernet2.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ typedef int socklen_t;
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#ifdef __FreeBSD__
#include <netinet/in.h> // sockaddr_in.
Copy link
Owner

Choose a reason for hiding this comment

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

no need of the ifdef, but this has to go to AppleWin

#endif

#endif

Expand Down
3 changes: 3 additions & 0 deletions source/linux/network/portfwds.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
#include <sys/socket.h>
#include <netinet/in.h>
#include <netdb.h>
#ifdef __FreeBSD__
#include <netinet/in.h> // sockaddr_in.
Copy link
Owner

Choose a reason for hiding this comment

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

will commit

#endif

namespace
{
Expand Down