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

nasm - upgrade to conan v2, based on prior work done by @czoido #13485

Merged
merged 17 commits into from
Oct 26, 2022

Conversation

paulharris
Copy link
Contributor

nasm wouldn't build for me as I'm using the "msvc" compiler, not the "Visual Studio" compiler.
So I thought I'd upgrade to conan v2 at the same time, based on the work found here:
https://github.com/czoido/conan-center-index/blob/master/recipes/nasm/all/conanfile.py
mentioned here: conan-io/conan#11751


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the conan-center hook activated.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@paulharris
Copy link
Contributor Author

Note: rolled back to #13485 (comment)
I have no idea re this error:

Configuring environment variables
nasm/2.15.05: 
configure.ac:1: error: possibly undefined macro: dnl
      If this token and others are legitimate, please use m4_pattern_allow.
      See the Autoconf documentation.
configure.ac:62: error: possibly undefined macro: AC_DEFINE
configure.ac:107: error: possibly undefined macro: AC_CHECK_PROGS
autoreconf: /usr/bin/autoconf failed with exit status: 1
nasm/2.15.05: WARN: Using the new toolchains and generators without specifying a build profile (e.g: -pr:b=default) is discouraged and might cause failures and unexpected behavior
nasm/2.15.05: ERROR: Package '24647d9fe8ec489125dfbae4b3ebefaf7581674c' build failed
nasm/2.15.05: WARN: Build folder /home/conan/w/prod/BuildSingleReference/.conan/data/nasm/2.15.05/_/_/build/24647d9fe8ec489125dfbae4b3ebefaf7581674c/build-release
ERROR: nasm/2.15.05: Error in build() method, line 96
	autotools.autoreconf()
	ConanException: Error 1 while executing autoreconf '--force' '--install'

@conan-center-bot

This comment has been minimized.

@paulharris
Copy link
Contributor Author

I'm not sure what to do about the errors in the build... help please?

paulharris and others added 2 commits October 17, 2022 23:51
Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>
Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>
@conan-center-bot

This comment has been minimized.

@paulharris
Copy link
Contributor Author

Help please, I don't understand why this failed... looks like python failed to substitute asm_file variable contents into f"etcetc {asm_file}" ... ?

@conan-center-bot

This comment has been minimized.

@paulharris
Copy link
Contributor Author

@czoido how does this look now?

recipes/nasm/all/conanfile.py Outdated Show resolved Hide resolved
recipes/nasm/all/test_package/conanfile.py Outdated Show resolved Hide resolved
recipes/nasm/all/test_package/conanfile.py Outdated Show resolved Hide resolved
recipes/nasm/all/test_package/conanfile.py Outdated Show resolved Hide resolved
recipes/nasm/all/test_package/conanfile.py Outdated Show resolved Hide resolved
recipes/nasm/all/test_package/conanfile.py Outdated Show resolved Hide resolved
recipes/nasm/all/test_v1_package/conanfile.py Outdated Show resolved Hide resolved
recipes/nasm/all/test_package/CMakeLists.txt Outdated Show resolved Hide resolved
recipes/nasm/all/conanfile.py Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

Questions and some cleanup :)

recipes/nasm/all/conanfile.py Outdated Show resolved Hide resolved
replace_in_file(self, "Makefile", "-Werror=attributes", "")

# Need "-arch" flag for the linker when cross-compiling.
# FIXME: Revisit after https://github.com/conan-io/conan/issues/9069, using new Autotools integration
Copy link
Contributor

Choose a reason for hiding this comment

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

this issue is closed, is this code still required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, it is hard for me to comment on or test as I don't cross-compile locally.
Also, I see there are a bunch of PRs open on AutoTools so I didn't want to test anything related to AutoTools specifically as whatever problems I hit may already be in the pipeline...

recipes/nasm/all/conanfile.py Outdated Show resolved Hide resolved
recipes/nasm/all/conanfile.py Outdated Show resolved Hide resolved
recipes/nasm/all/conanfile.py Outdated Show resolved Hide resolved
recipes/nasm/all/conanfile.py Outdated Show resolved Hide resolved
recipes/nasm/all/conanfile.py Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

recipes/nasm/all/conanfile.py Outdated Show resolved Hide resolved
recipes/nasm/all/conanfile.py Outdated Show resolved Hide resolved
Comment on lines +99 to +100
shutil.copy2("nasm.exe", "nasmw.exe")
shutil.copy2("ndisasm.exe", "ndisasmw.exe")
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes well I don't understand how I'd do this with conan.tools.files.copy(),
I'd have to copy to a temporary folder and then rename into the right place, and then delete the temp folder?
Seems crazy?

I think we should:

  • leave it there, with a FIXME or something
  • add a conan.tools.files.copy_file() to 1.54
  • add a linter check for shutil.copy() and shutil.copy2()

Copy link
Contributor

Choose a reason for hiding this comment

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

That does sound crazy 🤪 lol sorry I didn't realize it was not available 😅

recipes/nasm/all/conanfile.py Outdated Show resolved Hide resolved
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
@conan-center-bot
Copy link
Collaborator

All green in build 18 (0bd8f0d4523d2dc63d2dd1bc4b916204ca599578):

  • nasm/2.15.05@:
    All packages built successfully! (All logs)

  • nasm/2.13.01@:
    All packages built successfully! (All logs)

  • nasm/2.14@:
    All packages built successfully! (All logs)

  • nasm/2.13.02@:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit 720a0a9 into conan-io:master Oct 26, 2022
@paulharris paulharris deleted the nasm-build-for-msvc branch October 26, 2022 13:12
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

Successfully merging this pull request may close these issues.

5 participants