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

wants to merge 1 commit into from

Conversation

kiyolee
Copy link

@kiyolee kiyolee commented Jun 1, 2024

Some tweaks to get APPLEN target to build on FreeBSD.
Not yet verify if the build actually works on FreeBSD.

audetto
audetto reviewed Jun 1, 2024
View reviewed changes
source/CMakeLists.txt
# 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>.

audetto
audetto reviewed Jun 1, 2024
View reviewed changes
source/Tfe/DNS.cpp
@@ -25,6 +25,9 @@ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
#ifndef _MSC_VER
#include <arpa/inet.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.

audetto
audetto reviewed Jun 1, 2024
View reviewed changes
source/linux/network/portfwds.cpp
@@ -7,6 +7,9 @@
#include <sys/types.h>
#include <sys/socket.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

audetto
audetto reviewed Jun 1, 2024
View reviewed changes
source/Uthernet2.cpp
@@ -75,6 +75,9 @@ typedef int socklen_t;
#include <fcntl.h>
#include <sys/types.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

@kiyolee
Copy link
Author

kiyolee commented Jun 1, 2024

FYI: APPLEN does run on FreeBSD, at least can boot DOS 3.3.

@audetto
Copy link
Owner

audetto commented Jun 2, 2024

Hi

the minizip issue is odd. I have found an alternative solution.
It is not as stable as yours, but I can pretend it does not exist.

https://github.com/audetto/AppleWin/tree/freebsd2

I will try to upstream it to AppleWin, they should not care.

I have opened an issue madler/zlib#977, feel free to chip in.

Then, as you say it works, but the ncurse F scan codes must be all over the place.

@audetto
Copy link
Owner

audetto commented Jun 2, 2024

Actually it is Shift-F behaving oddly, so I added F4 to exit.

@kiyolee
Copy link
Author

kiyolee commented Jun 2, 2024

An alternative solution is to drop minizip detection at all on FreeBSD. Very likely /usr/local/include would already be in the include path and #include <minizip/zip.h> might just work. That may work for Linux too as /usr/include is always there in the include path anyway.

@kiyolee
Copy link
Author

kiyolee commented Jun 2, 2024

Actually it is Shift-F behaving oddly, so I added F4 to exit.

I could only Ctrl+\ (Quit signal) to exit which is not nice.

@audetto
Copy link
Owner

audetto commented Jun 4, 2024

The most elegant solution would be to move minizip down 1 level inside AW, so that we all include it the same.

I see you have updated it recently, do you fancy proposing a new PR upstream?

@kiyolee kiyolee force-pushed the applen-freebsd-build branch 2 times, most recently from dc7c570 to 240ec92 Compare October 17, 2024 07:46
@audetto
Copy link
Owner

audetto commented Nov 3, 2024

minizip has removed the source of ambiguity.

madler/zlib@7e6f078

so, I will try to change AppleWin upstream for the new layout of minizip, otherwise I will just incorporate these changes. directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@audetto audetto audetto left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kiyolee @audetto