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

FreeBSD Ports exp-run results #70

Closed
ghost opened this issue Apr 8, 2021 · 12 comments
Closed

FreeBSD Ports exp-run results #70

ghost opened this issue Apr 8, 2021 · 12 comments

Comments

@ghost
Copy link

ghost commented Apr 8, 2021

Hi,

the first round of the exp-run has finished. There were only a handful of failures.

  1. Our ninja package depends on Python and unsurprisingly there were many issues related to underspecified build dependencies as a result since samurai has no dependencies itself.

  2. Many build order bugs. I tried addressing a few:

    Remaining issues are

  3. One self-made issue with argument order in our framework. -v was always added as the last argument to Ninja but thanks to SAMUFLAGS we do not even need to do this with Samurai.

  4. A samurai/ninja behavior difference in sysutils/fluxengine. It has its own Ninja generator which creates a build file with extra whitespace. Ninja accepts it but Samurai does not. This was easy enough to fix but I wonder if this could be considered a bug in Samurai. Please see the commit for more details: freebsd/freebsd-ports@7b1d905

freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this issue Apr 8, 2021
Uncovered while building with devel/samurai but can probably also
happen with ninja eventually.  There are some missing dependencies
on the components module.

In file included from /wrkdirs/usr/ports/audio/muse-sequencer/work/muse-muse_3_1_1/muse3/muse/waveedit/wavecanvas.cpp:72:
/wrkdirs/usr/ports/audio/muse-sequencer/work/muse-muse_3_1_1/muse3/muse/components/copy_on_write.h:26:10: fatal error: 'ui_copy_on_write_base.h' file not found
 #include "ui_copy_on_write_base.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~

http://package18.nyi.freebsd.org/data/122amd64-default-foo/2021-04-05_21h03m50s/logs/errors/muse-sequencer-3.1.1_1.log

michaelforney/samurai#70

PR:		254678
Obtained from:	upstream (midieedit, waveedit patches)
@michaelforney
Copy link
Owner

Thanks for the update! That sounds really promising. I'm really happy to see FreeBSD consider using samurai since you have such a large ports tree. It should really open the door for other operating systems to try it as well.

  1. Many build order bugs. I tried addressing a few:

Do you have plans to submit these patches upstream? It would be quite nice to get these sort of issues fixed for the benefit of everyone using samurai.

If you have any doubts about any of the patches, let me know and I can try to take a look.

  1. A samurai/ninja behavior difference in sysutils/fluxengine. It has its own Ninja generator which creates a build file with extra whitespace. Ninja accepts it but Samurai does not. This was easy enough to fix but I wonder if this could be considered a bug in Samurai. Please see the commit for more details: freebsd/freebsd-ports@7b1d905

I pushed a fix for this in aff2085, let me know if that works. I wish the ninja manual was a bit more detailed about the syntax :/

@ghost
Copy link
Author

ghost commented Apr 9, 2021

Do you have plans to submit these patches upstream?

If you have any doubts about any of the patches, let me know and I can try to take a look.

I have doubts about all of the patches but let's see what the upstreams say.

I have some trouble figuring out what the fbthrift issue even could be. It's a maze of CMake build files. I also cannot reproduce the failure on my machines...

I pushed a fix for this in aff2085, let me know if that works. I wish the ninja manual was a bit more detailed about the syntax :/

Yes, thanks. That seems to work fine.

freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this issue Apr 9, 2021
This allows samurai to accept stray indented but otherwise empty
lines and we can drop the fluxengine workaround.

michaelforney/samurai#70
@michaelforney
Copy link
Owner

fbthrift bug should be fixed by facebook/fbthrift#422

Is it just bijiben that is left after this?

@michaelforney
Copy link
Owner

gnome-notes (bijiben) pull request submitted: https://gitlab.gnome.org/GNOME/gnome-notes/-/merge_requests/116

freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this issue Apr 10, 2021
…riftcpp2 target (D29353)

One source file in this target, async/HeaderClientChannel.cpp,
depends on the generated header RocketUpgradeAsyncClient.h, so it
needs to depend on the target that generates this header.

This causes a build error with samurai due to an incorrect build
order, and can be reproduced with ninja as well by building
thrift/lib/cpp2/CMakeFiles/thriftcpp2.dir/async/HeaderClientChannel.cpp.o
directly with an empty .ninja_deps.

michaelforney/samurai#70

PR:		254678
freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this issue Apr 10, 2021
…ncy sources (D29353)

The meson manual states that

> Each target that depends on a generated header should add that
> header to it's sources, as seen above with libfoo and myexe. This
> is because there is no way for Meson or the backend to know that
> myexe depends on foo.h just because libfoo does, it could be a
> private header.

Since biji-marshalers.h is included by the libbiji public header
libbiji.h, it must be added as a source to all targets that depend
on libbiji. This can be done by adding it to the sources of
libbiji_dep.

This should unbreak the build with samurai.

michaelforney/samurai#70

PR:		254678
@ghost
Copy link
Author

ghost commented Apr 10, 2021

Thank you. I believe that bijiben was the last failure. I've applied your patches to the ports tree and have asked for another exp-run round.

@ghost
Copy link
Author

ghost commented Apr 10, 2021

I've noticed something. qt5-webengine's configure fails to recognize samurai as valid system ninja and builds a bundled ninja(!).

The check goes like this (configure.pri line 163):

defineTest(qtConfTest_detectNinja) {
    ninja = $$qtConfFindInPath("ninja$$EXE_SUFFIX")
    !isEmpty(ninja) {
        qtLog("Found ninja from path: $$ninja")
        qtRunLoggedCommand("$$ninja --version", version)|return(false)
        contains(version, "1\.([7-9]|1[0-9])\..*"): return(true)
        qtLog("Ninja version too old")
    }
    qtLog("Building own ninja")
    return(false)
}

It tries to run ninja --version and expects it to return a 3 point version which samu does not do.

$ samu --version
1.9

$ ninja --version
1.10.2

If I patch samu to return 1.9.0 instead it passes the check. Is it possible to change this?

@michaelforney
Copy link
Owner

If I patch samu to return 1.9.0 instead it passes the check. Is it possible to change this?

Done in ca5a6ba.

freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this issue Apr 10, 2021
This makes --version return 1.9.0 instead of 1.9.  qt5-webengine's
configure checks for system ninja and tries to parse --version and
expects it to have 3 components.  Without this qt5-webengine falls
back to using a bundled copy of ninja instead.

michaelforney/samurai#70

PR:		254678
fluffykhv pushed a commit to fluffykhv/freebsd-ports that referenced this issue Apr 10, 2021
This makes --version return 1.9.0 instead of 1.9.  qt5-webengine's
configure checks for system ninja and tries to parse --version and
expects it to have 3 components.  Without this qt5-webengine falls
back to using a bundled copy of ninja instead.

michaelforney/samurai#70

PR:		254678
@ghost
Copy link
Author

ghost commented Apr 13, 2021

The exp-run has finished and it looks good, only one new failure in deno. Something wrong with the bundled v8 build. Not sure what's going one with that. I might just hardcode ninja there for now to be done with it.
http://package18.nyi.freebsd.org/data/122amd64-default-foo/2021-04-10_08h07m32s/logs/errors/deno-1.8.2_1.log

On a more general note I noticed that some escape sequences for color compiler diagnostics make it through to the build logs. I disabled that for Meson builds by passing -Db_colorout=never to them as per Samurai's README.md but many CMake or GN builds force on colors and every project does it slightly differently without any setting that I could find to disable that globally. :-( This isn't terribly important but do you have any advice what can be done here if anything? I realize that from samu's side there probably cannot be done much about it.

arrowd pushed a commit to freebsd/freebsd-ports-haskell that referenced this issue Apr 13, 2021
Uncovered while building with devel/samurai but can probably also
happen with ninja eventually.  There are some missing dependencies
on the components module.

In file included from /wrkdirs/usr/ports/audio/muse-sequencer/work/muse-muse_3_1_1/muse3/muse/waveedit/wavecanvas.cpp:72:
/wrkdirs/usr/ports/audio/muse-sequencer/work/muse-muse_3_1_1/muse3/muse/components/copy_on_write.h:26:10: fatal error: 'ui_copy_on_write_base.h' file not found
 #include "ui_copy_on_write_base.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~

http://package18.nyi.freebsd.org/data/122amd64-default-foo/2021-04-05_21h03m50s/logs/errors/muse-sequencer-3.1.1_1.log

michaelforney/samurai#70

PR:		254678
Obtained from:	upstream (midieedit, waveedit patches)
arrowd pushed a commit to freebsd/freebsd-ports-haskell that referenced this issue Apr 13, 2021
This allows samurai to accept stray indented but otherwise empty
lines and we can drop the fluxengine workaround.

michaelforney/samurai#70
arrowd pushed a commit to freebsd/freebsd-ports-haskell that referenced this issue Apr 13, 2021
…riftcpp2 target (D29353)

One source file in this target, async/HeaderClientChannel.cpp,
depends on the generated header RocketUpgradeAsyncClient.h, so it
needs to depend on the target that generates this header.

This causes a build error with samurai due to an incorrect build
order, and can be reproduced with ninja as well by building
thrift/lib/cpp2/CMakeFiles/thriftcpp2.dir/async/HeaderClientChannel.cpp.o
directly with an empty .ninja_deps.

michaelforney/samurai#70

PR:		254678
arrowd pushed a commit to freebsd/freebsd-ports-haskell that referenced this issue Apr 13, 2021
…ncy sources (D29353)

The meson manual states that

> Each target that depends on a generated header should add that
> header to it's sources, as seen above with libfoo and myexe. This
> is because there is no way for Meson or the backend to know that
> myexe depends on foo.h just because libfoo does, it could be a
> private header.

Since biji-marshalers.h is included by the libbiji public header
libbiji.h, it must be added as a source to all targets that depend
on libbiji. This can be done by adding it to the sources of
libbiji_dep.

This should unbreak the build with samurai.

michaelforney/samurai#70

PR:		254678
arrowd pushed a commit to freebsd/freebsd-ports-haskell that referenced this issue Apr 13, 2021
This makes --version return 1.9.0 instead of 1.9.  qt5-webengine's
configure checks for system ninja and tries to parse --version and
expects it to have 3 components.  Without this qt5-webengine falls
back to using a bundled copy of ninja instead.

michaelforney/samurai#70

PR:		254678
@michaelforney
Copy link
Owner

The exp-run has finished and it looks good, only one new failure in deno. Something wrong with the bundled v8 build. Not sure what's going one with that. I might just hardcode ninja there for now to be done with it.
http://package18.nyi.freebsd.org/data/122amd64-default-foo/2021-04-10_08h07m32s/logs/errors/deno-1.8.2_1.log

Though this looks like the same type of error as other build-order issues (missing header error), I am a bit suspicious about this one since src/objects/function-syntax-kind.h is not a generated header, it is tracked in source control. Additionally, the appropriate include directory appears on the command-line: -I../../../deno-1.8.2/cargo-crates/rusty_v8-0.21.0/v8

Additionally, we have these lines

warning: c++: error: no such file or directory: 'src/vendor/SPIRV-Cross/spirv_cross.cpp'
warning: c++: error: no input files
warning: c++: error: no such file or directory: 'src/vendor/SPIRV-Cross/spirv_cross_parsed_ir.cpp'
warning: c++: error: no input files

The following warnings were emitted during compilation:

warning: cc: error: no such file or directory: 'c/freebsd.c'
warning: cc: error: no input files

Are you sure that the build with ninja does not have these problems? Can you reproduce the failure and poke around the build directory? I don't seem to be able to reproduce it locally when building rusty_v8 (but I tested on Linux since I don't have a FreeBSD install and the build took too long to run on sourcehut).

On a more general note I noticed that some escape sequences for color compiler diagnostics make it through to the build logs. I disabled that for Meson builds by passing -Db_colorout=never to them as per Samurai's README.md but many CMake or GN builds force on colors and every project does it slightly differently without any setting that I could find to disable that globally. :-( This isn't terribly important but do you have any advice what can be done here if anything? I realize that from samu's side there probably cannot be done much about it.

Sorry, I'm not sure. It seems these projects are hard-coding -fdiagnostics-color=always into their build files. Can you pipe the output into some tool/script that strips the escape sequences?

@ghost
Copy link
Author

ghost commented Apr 20, 2021

I went ahead and committed samurai support to the ports tree. It can be switched on by having DEFAULT_VERSIONS+=ninja=samurai in make.conf now.

Can you reproduce the failure and poke around the build directory?

I have been unable to reproduce it myself. In the mean time deno was also updated, so maybe the problem is already gone (but rusty_v8 wasn't updated so probably not). I've asked for a rebuilt on the package builder and for a copy of the working directory in case it fails again.

Can you pipe the output into some tool/script that strips the escape sequences?

Hmm, I'm not sure this is worth the trouble it might cause. It also feels like solving the problem at the wrong layer. What probably should happen is to implement something like b_colorout in CMake . It's just how it is at the moment then. The logs can be viewed with less -R anyway.

@ghost
Copy link
Author

ghost commented Apr 21, 2021

I think that's all. Thanks for all your help.

@ghost ghost closed this as completed Apr 21, 2021
@michaelforney
Copy link
Owner

Thank you for tracking down all those failing packages :)

This issue was closed.
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

No branches or pull requests

1 participant