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

Switch to FreeBSD 13.2 #1403

Merged
merged 3 commits into from
Jan 9, 2024
Merged

Switch to FreeBSD 13.2 #1403

merged 3 commits into from
Jan 9, 2024

Conversation

ydirson
Copy link
Contributor

@ydirson ydirson commented Jan 4, 2024

Rerolling #1401 following GH closing it on push mistake - :sigh:

@ydirson ydirson requested a review from a team as a code owner January 4, 2024 16:45
@Emilgardis Emilgardis linked an issue Jan 4, 2024 that may be closed by this pull request
@ydirson ydirson force-pushed the freebsd13 branch 5 times, most recently from e5a7c05 to 4ffdb17 Compare January 5, 2024 16:32
Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

Nicely done!

Can you leave out 8a87c7c please

using ARG was done on purpose, see #593 and for example https://github.com/cross-rs/cross/wiki/Recipes#dockerfile where we recommend setting it, I do agree with changing it though, making it propagate into the images probably makes sense

.changes/1390.json Outdated Show resolved Hide resolved
Comment on lines 34 to 37
# symlinks so toolchain will find headers and libs
RUN mkdir /usr/local/x86_64-unknown-freebsd13/usr
RUN ln -s ../include /usr/local/x86_64-unknown-freebsd13/usr/include
RUN ln -s ../lib /usr/local/x86_64-unknown-freebsd13/usr/lib
Copy link
Member

Choose a reason for hiding this comment

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

which toolchain? gcc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depending on what bindgen spawns, it could be clang for what I know - that's the issue we talked about a few weeks ago

Copy link
Member

Choose a reason for hiding this comment

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

Ah!

I'm still unsure if this is actually the correct approach (specifically for include), I think something like #1389 would be better

The reason I think it's incorrect is because there's different expectations here on what a sysroot actually is. The approach ubuntu seems to have is that headers that are common between platforms live in the system include folder, gcc/g++ uses /usr/aarch64-linux-gnu.

I do think though that this is the wrong approach.
First I'd want a absolute path instead of a relative one (I don't even understand what .. refers to here)
Second, if .. is /usr, this is incredibly wrong as .a/.so in /usr/lib are not correct for freebsd

I think I'd rather see a more fleshed out approach to this rather than a link.

Copy link
Member

@Emilgardis Emilgardis Jan 5, 2024

Choose a reason for hiding this comment

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

However, I'm fine with a link if the path is spelled out explicitly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, I'm fine with a link if the path is spelled out explicitly

I'm not sure I get what you mean here.

I don't think the approach of Ubuntu (which, indeed, is Debian's, as Ubuntu only inherits it) is fundamentally different from the rest. What is important, is that the compiler MUST find suitable system headers and libs, ie. it SHOULD NOT require any special flags to find them (once the sysroot is set, if we even have to specify this one). And AFAICT when we're using the (cross-)compilers and (foreign) libs provided by the distro, there is just no need to set them or even set the sysroot, which is / for their supported cross targets.

Here the situation is different: we have a sysroot with an uncommon directory layout (noone installs their headers in /include/) and unsurprisingly this causes problems to the default compiler setup.

In fact I just realize that it is our freebsd.sh which breaks those assumptions by extracting /usr/include and /usr/lib just to install them as /include and /lib in the sysroot! 🤯
Can't see any justification for this non-standard behaviour in the commits introduing it, will try and replace that commit with a correction of offending processing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why, that will naturally be /usr/local/x86_64-unknown-freebsd13/lib, and it is usually considered best practice to use relative links, so they do not get broken when (part of) the fs gets mounted differently.

Copy link
Member

Choose a reason for hiding this comment

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

I forgot about the special nature of ln and how it resolves (or rather doesnt resolve) paths :)

Just for my own sake, can we add the comment

# symlink `/usr/local/x86_64-unknown-freebsd13/{include,lib}` into `/usr/local/x86_64-unknown-freebsd13/usr`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I'm just going to simplify this mess by simply shipping headers and libs from base.txz and drop all this toying around.

Turns out things are not that simple: while apparently clang uses standard paths, the cross-compiled gcc does indeed want then in this strange location, which kind of explains the whole mess:

/tmp/tmp.ZHQ6toFJwz/gcc-build/./gcc/xgcc -B/tmp/tmp.ZHQ6toFJwz/gcc-build/./gcc/ -B/usr/local/x86_64-unknown-freebsd13/bin/ -B/usr/local/x86_64-unknown-freebsd13/lib/ -isystem /usr/local/x86_64-unknown-freebsd13/
include -isystem /usr/local/x86_64-unknown-freebsd13/sys-include    -g -O2 -O2  -g -O2 -DIN_GCC  -DCROSS_DIRECTORY_STRUCTURE  -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wstrict-prototypes -Wmissing-pro
totypes -Wold-style-definition  -isystem ./include   -fpic -pthread -g -DIN_LIBGCC2 -fbuilding-libgcc -fno-stack-protector -Dinhibit_libc  -fpic -pthread -I. -I. -I../.././gcc -I../../../gcc/libgcc -I../../../gc
c/libgcc/. -I../../../gcc/libgcc/../gcc -I../../../gcc/libgcc/../include  -DHAVE_CC_TLS  -o _negdi2.o -MT _negdi2.o -MD -MP -MF _negdi2.dep -DL_negdi2 -c ../../../gcc/libgcc/libgcc2.c -fvisibility=hidden -DHIDE_
EXPORTS
mv tmp-libgcc.map.in libgcc.map.in
In file included from ../../../gcc/libgcc/../gcc/tsystem.h:44:0,
                 from ../../../gcc/libgcc/libgcc2.c:27:
/tmp/tmp.ZHQ6toFJwz/gcc-build/gcc/include/stddef.h:56:24: fatal error: sys/_types.h: No such file or directory

I cannot see any cross-rs material causing gcc to use those paths, so that would be a gcc default? First occurrence in build happens in libcc1/ (though this dir does build successfully, only libgcc)):

checking for x86_64-unknown-freebsd13-gcc... /tmp/tmp.ZHQ6toFJwz/gcc-build/./gcc/xgcc -B/tmp/tmp.ZHQ6toFJwz/gcc-build/./gcc/ -B/usr/local/x86_64-unknown-freebsd13/bin/ -B/usr/local/x86_64-unknown-freebsd13/lib/ 
-isystem /usr/local/x86_64-unknown-freebsd13/include -isystem /usr/local/x86_64-unknown-freebsd13/sys-include   

Digging....

Copy link
Contributor Author

@ydirson ydirson Jan 9, 2024

Choose a reason for hiding this comment

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

I forgot about the special nature of ln and how it resolves (or rather doesnt resolve) paths :)

Just for my own sake, can we add the comment

Alternatively we could use ln -sr so the paths are more self-explaining but still end up with the same relative result, if we deem it acceptable to have 2 toolchains with different search paths and just use the symlinks to make up for that.

Which I'm already starting consider to be a better idea than digging through the automess stuff: from the wonderful generated 1980-compatible sh code I can only deduce libgcc/configure gets fed a CC with those flags by some other build step. I can see this set by recursive calls to make from libgcc/Makefile.in for multilib builds, but this value is filled by their gcc/configure itself 🤯

Will stop wasting time and get back to symlinks, but that still implies merging /lib and /usr/lib, which I don't like at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, that commit does not address a new issue, so I'm pulling it off this PR and will open a separate one

@ydirson
Copy link
Contributor Author

ydirson commented Jan 5, 2024

Nicely done!

Can you leave out 8a87c7c please

using ARG was done on purpose, see #593 and for example https://github.com/cross-rs/cross/wiki/Recipes#dockerfile where we recommend setting it, I do agree with changing it though, making it propagate into the images probably makes sense

I see this comes from the exact same reason I'm proposing this change.

The problem I see is that ARG is supposed to set a var that's local to the recipe, and no dockerfile reads it, so as it stands it seems to have no effect (according to the spec). So maybe docker and podman interpret the thing differently, which would likely be a bug in one or more of docker|podman|docker's doc.

We can add an ENV statement in addition to ARG, but I don't see the point if all we want is to add this var to the env, that's what ENV is for.

@Emilgardis
Copy link
Member

Emilgardis commented Jan 5, 2024

My original review message got removed so I just quickly rewrote it, what I meant to add to my sentiment about 8a87c7c is that it should be a separate pr!

ARG is in dockerfiles seen by RUN

@ydirson
Copy link
Contributor Author

ydirson commented Jan 7, 2024

My original review message got removed so I just quickly rewrote it, what I meant to add to my sentiment about 8a87c7c is that it should be a separate pr!

👍

ARG is in dockerfiles seen by RUN

This says the variables are available in the RUN command itself, not that they are added to the RUN commands' environment. Especially this statement and example make it clear:

Using the example above but a different ENV specification you can create more useful interactions between ARG and ENV instructions:

FROM ubuntu
ARG CONT_IMG_VER
ENV CONT_IMG_VER=${CONT_IMG_VER:-v1.0.0}
RUN echo $CONT_IMG_VER

ENV CONT_IMG_VER=${CONT_IMG_VER:-v1.0.0} would not have any usefulness above ENV CONT_IMG_VER=v1.0.0.

To me:

  • ARG is clearly to allow customization of the build using --build-arg, and I don't see that we need it for DEBIAN_FRONTEND
  • ENV is clearly to set an env variable, and that one we do need
  • so it would appear that Docker has an undocumented and likely unwanted "feature" that ARG vars are also promoted to ENV?

@Emilgardis
Copy link
Member

I wouldn't say it's unwanted, I've seen it used in other images for non-persistant environment variables. Anyway, I think we can drop the subject about ARG/ENV and just make the change in another pr :D

@Emilgardis Emilgardis added CI-aarch64-unknown-freebsd Run CI for aarch64-unknown-freebsd target CI-i686-unknown-freebsd Run CI for i686-unknown-freebsd target CI-x86_64-unknown-freebsd Run CI for x86_64-unknown-freebsd target labels Jan 9, 2024
12.4 is EOL so we must move on.  We keep the toolchain version for now,
but it is very old, and seems to produce many more warnings during the
build.
This version of binutils would fail to configure if `makeinfo` is not
available (despite a notice that the only impact ought to be not installing
some documentation).  Looks like a binutils regression.
bash functions require that /bin/sh is bash, or that SHELL is set to bash
in the Dockerfile, but the former wastes resources and the latter clashes
with OCI containers apparently not supporting SHELL.

Calling the functionality as real scripts avoids the issue.
@Emilgardis
Copy link
Member

Emilgardis commented Jan 9, 2024

Is this ready?

/ci try

edit: ugh, why does gha do this.....

@ydirson
Copy link
Contributor Author

ydirson commented Jan 9, 2024

Is this ready?

I'd think so

@Emilgardis
Copy link
Member

/ci try

This comment has been minimized.

Copy link

github-actions bot commented Jan 9, 2024

Try run for comment

Successful Jobs

Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

Thank you! Looking forward to other potential prs :)

@Emilgardis Emilgardis added this pull request to the merge queue Jan 9, 2024
Merged via the queue into cross-rs:main with commit 78c0117 Jan 9, 2024
21 checks passed
@ydirson ydirson deleted the freebsd13 branch January 11, 2024 10:06
@Emilgardis Emilgardis added this to the v0.3.0 milestone Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-aarch64-unknown-freebsd Run CI for aarch64-unknown-freebsd target CI-i686-unknown-freebsd Run CI for i686-unknown-freebsd target CI-x86_64-unknown-freebsd Run CI for x86_64-unknown-freebsd target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FreeBSD 12 is EoL, update to 13
2 participants