fix byte_for_jsoo, fix #5282

[hhugo]

No description provided.

[hhugo]

@emillon, can you check this fixes your issue.

[hhugo]

cc @nojb

[nojb]

@hhugo thanks for the ping, I will try to try this out today or tomorrow.

[emillon]

@emillon, can you check this fixes your issue.

Yes, that works! thanks!

[hhugo]

Does anyone know the expected use-case for
(context (default (disable_dynamically_linked_foreign_archives true))) ? Is it expected that libraries compiled with this mode still mention a dynamic library in the cma archive ?

[nojb]

Does anyone know the expected use-case for (context (default (disable_dynamically_linked_foreign_archives true))) ?

No idea. For sure @jeremiedimino knows, but he is on holidays until 2022.

@emillon, can you check this fixes your issue.

Yes, that works! thanks!

Following this confirmation, is there anything left to check for this PR @hhugo?

[hhugo]

I think this PR should be merged because it fixes a regression.
But the test also shows that js compilation is broken when disable_dynamically_linked_foreign_archives=true (which #5049 was trying to fix/implement). I'd be nice to know whether js compilation is broken on platform without dynamic library support.

[ghost]

Does anyone know the expected use-case for
(context (default (disable_dynamically_linked_foreign_archives true))) ? Is it expected that libraries compiled with this mode still mention a dynamic library in the cma archive ?

The original PR to add this flag doesn't state a reason (#2864), however the timing seems to correspond with the time we were trying to use the upstream Dune rules to build Jane Street codebase. So it's well possible we added this simply because we needed it to build Jane Street code base.

[hhugo]

I've rebased the branch and signed the commit, it should be ready to be merged.