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

[jemalloc|vcpkg_configure_make] update and some script fixes #25009

Merged
merged 20 commits into from
Aug 19, 2022

Conversation

Neumann-A
Copy link
Contributor

lets rebuild the world ;)

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/jemalloc/vcpkg.json

Valid values for the license field can be found in the documentation

@Neumann-A
Copy link
Contributor Author

Current CI baseline:

jemalloc:arm64-windows=fail
jemalloc:arm-uwp=fail
jemalloc:x64-linux=fail
jemalloc:x64-osx=fail
jemalloc:x64-uwp=fail
jemalloc:x64-windows-static=fail

ports/jemalloc/portfile.cmake Outdated Show resolved Hide resolved
ports/jemalloc/portfile.cmake Outdated Show resolved Hide resolved
@Adela0814 Adela0814 added the category:port-update The issue is with a library, which is requesting update new revision label May 31, 2022
ports/jemalloc/vcpkg.json Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_configure_make.cmake Outdated Show resolved Hide resolved
@Neumann-A
Copy link
Contributor Author

@BillyONeal: rsocket ICEd here. Please check the baseline.

@BillyONeal
Copy link
Member

@BillyONeal: rsocket ICEd here. Please check the baseline.

#24959

@Neumann-A Neumann-A marked this pull request as ready for review June 7, 2022 10:24
@JackBoosY JackBoosY added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Jun 8, 2022
scripts/cmake/vcpkg_configure_make.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_configure_make.cmake Show resolved Hide resolved
@Neumann-A Neumann-A dismissed stale reviews from JackBoosY and github-actions[bot] via 5e2d82e July 12, 2022 09:02
github-actions[bot]
github-actions bot previously approved these changes Jul 12, 2022
z_vcpkg_append_to_configure_environment(configure_env CPP_FOR_BUILD "compile ${VCPKG_DETECTED_CMAKE_C_COMPILER} -E")
z_vcpkg_append_to_configure_environment(configure_env CXX_FOR_BUILD "compile ${VCPKG_DETECTED_CMAKE_CXX_COMPILER}")
else()
# Silly trick to make configure accept CC_FOR_BUILD but in reallity CC_FOR_BUILD is deactivated.
Copy link
Member

Choose a reason for hiding this comment

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

I'm going to need more explanation on these lines. How does this trick work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

configure wants the following:
a) a successful return code for the compile | true (true is a binary, same as false; available via msys on windows)
b) an output file. Depending on the test it either wants just an object file or an executable. (which is why there are two touch commands)

Be aware that the trick is already used in:

if(VCPKG_CROSSCOMPILING)
# Silly trick to make configure accept CC_FOR_BUILD but in reallity CC_FOR_BUILD is deactivated.
set(ENV{CC_FOR_BUILD} "touch a.out | touch conftest${VCPKG_HOST_EXECUTABLE_SUFFIX} | true")
set(ENV{CPP_FOR_BUILD} "touch a.out | touch conftest${VCPKG_HOST_EXECUTABLE_SUFFIX} | true")
endif()

I am just generalizing it here for all ports.

Copy link
Contributor

Choose a reason for hiding this comment

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

the trick made gmp fail for me, but @Neumann-A was not able to replicate the issue.
For me the issue was that the executable was not a real working one (while CC_FOR_BUILD should create an executable that the host IS able to run)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me the issue was that the executable was not a real working one (while CC_FOR_BUILD should create an executable that the host IS able to run)

The question is did it pass configure but failed the build or did it fail configure? I had the issue that the build was failing in libx11 due to that but the real reason was that this PR deactivates implicit cross builds in vcpkg by design. So extra steps have to be taken to re-enable those explicitly. (e.g. by copying the required host artifacts in the buildtree for the target so that make thinks it already build those and doesn't even try to build them. )

Copy link
Contributor

Choose a reason for hiding this comment

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

configure was failing

Copy link
Contributor

Choose a reason for hiding this comment

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

the script was not just checking the CC_FOR_BUILD returning true after trying to compile, but it even went on trying to run the simple executable produced, which failed with your trick

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be this one:

{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking for build system executable suffix" >&5
printf %s "checking for build system executable suffix... " >&6; }
if test ${gmp_cv_prog_exeext_for_build+y}
then :
  printf %s "(cached) " >&6
else $as_nop
  cat >conftest.c <<EOF
int
main ()
{
  return 0;
}
EOF
for i in .exe ,ff8 ""; do
  gmp_compile="$CC_FOR_BUILD conftest.c -o conftest$i"
  if { { eval echo "\"\$as_me\":${as_lineno-$LINENO}: \"$gmp_compile\""; } >&5
  (eval $gmp_compile) 2>&5
  ac_status=$?
  printf "%s\n" "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
  test $ac_status = 0; }; then
    if (./conftest) 2>&5; then
      gmp_cv_prog_exeext_for_build=$i
      break
    fi
  fi
done
rm -f conftest*
if test "${gmp_cv_prog_exeext_for_build+set}" != set; then
  as_fn_error $? "Cannot determine executable suffix" "$LINENO" 5
fi

was the only check i found trying to run the output. since the name of the variable is gmp_cv_prog_exeext_for_build it seems to be specific to gmp. So setting gmp_cv_prog_exeext_for_build by hand should fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes exactly. I was running on memory and didn't expect you to do the search work for me. Thanks for already finding it out

@JackBoosY
Copy link
Contributor

Ping @vicroms for review again.

Copy link
Member

@vicroms vicroms left a comment

Choose a reason for hiding this comment

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

LGTM but I'd like additional review @BillyONeal @ras0219-msft @dan-shaw @JavierMatosD

@vicroms vicroms added info:reviewed Pull Request changes follow basic guidelines and removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Aug 1, 2022
@JonLiu1993 JonLiu1993 added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines requires:author-response labels Aug 5, 2022
@m-kuhn
Copy link
Contributor

m-kuhn commented Aug 18, 2022

Are there any questions open regarding the vcpkg_configure_make parts or only the jemalloc parts?
The vcpkg_configure_make related changes are crucial for cross compilation and if these require less discussion they could be split into a separate PR to speed things up.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Aug 19, 2022
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

Thanks for explaining @Neumann-A

@JavierMatosD
Copy link
Contributor

Thanks!

@JavierMatosD JavierMatosD merged commit 624f1b4 into microsoft:master Aug 19, 2022
@Neumann-A Neumann-A deleted the update_jemalloc branch August 20, 2022 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.