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

Add support for py_modules_only compilation #483

Merged
merged 12 commits into from Aug 18, 2019

Conversation

@tommyli3318
Copy link
Member

commented Aug 10, 2019

What does this PR do?

Create support handling the case of compiling packages containing py_modules only. Currently, Nuitka assumes that a main package always exists and runs into an exception when it is not the case.

Why was it initiated? Any relevant Issues?

Issue #479
GSoC2019

PR Checklist

  • Correct base branch selected? develop for new features and bug fixes too.
    If it's part of a hotfix, it will be moved to master during it.
  • All tests still pass. Check the developer manual about Running the Tests.
    There are Travis tests that cover the most important things however, and you
    are welcome to rely on those, but they might not cover enough.
  • Ideally new features or fixed regressions ought to be covered via new tests.
  • Ideally new or changed features have documentation updates.
@tommyli3318

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2019

@kayhayen Need some pointers here
With the changes, python run_all.py only py_modules_only/ runs into the following exception:

Nuitka:WARNING:Recursed to module 'py_modules_only' at 'C:\Users\Tommy\Nuitka\tests\distutils\py_modules_only\build\lib\py_modules_only.py' twice.
Nuitka:INFO:Included compiled module 'py_modules_only'.
error: [WinError 3] The system cannot find the path specified: 'C:\\Users\\Tommy\\Nuitka\\tests\\distutils\\py_modules_only\\build\\lib\\py_modules_only'
Creating virtualenv for quick test:
Traceback (most recent call last):
  File "run_all.py", line 257, in <module>
    main()
  File "run_all.py", line 169, in main
    commands=['cd "%s"' % case_dir, "python setup.py bdist_nuitka"]
  File "C:\Users\Tommy\Nuitka\nuitka\tools\testing\Virtualenv.py", line 51, in runCommand
    assert os.system(command) == 0, command
AssertionError: call scripts\activate.bat && cd "C:\Users\Tommy\Nuitka\tests\distutils\py_modules_only" && python setup.py bdist_nuitka

It seems that py_modules_only is assumed to be a directory? How should I approach this?
python setup.py bdist_nuitka is the command which raised the error, is there a way to get a debug traceback so I know which part failed specifically?

@kayhayen

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

Well, I recommend you trace the command before executing it.

Telling Nuitka that a module should be included as a package, is bound for disaster. No wonder, it's not found, it's a file and not a directory.

On modules, use --include-module on packages use include-package, do not fix them. Probably the error messages from Nuitka are not good or not there? Not sure how robust Nuitka is against your errors there.

Yours,
Kay

@tommyli3318

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

@kayhayen
I changed the code to use only --include-module on py_modules. However I am still getting the same error. Not sure how to proceed from here.

Traceback:

Nuitka:WARNING:Recursed to module 'py_modules_only' at 'C:\Users\Tommy\Nuitka\tests\distutils\py_modules_only\build\lib\py_modules_only.py' twice.
Nuitka:INFO:Included compiled module 'py_modules_only'.
error: [WinError 3] The system cannot find the path specified: 'C:\\Users\\Tommy\\Nuitka\\tests\\distutils\\py_modules_only\\build\\lib\\py_modules_only'
Creating virtualenv for quick test:
Traceback (most recent call last):
  File "run_all.py", line 257, in <module>
    main()
  File "run_all.py", line 169, in main
    commands=['cd "%s"' % case_dir, "python setup.py bdist_nuitka"]
  File "C:\Users\Tommy\Nuitka\nuitka\tools\testing\Virtualenv.py", line 51, in runCommand
    assert os.system(command) == 0, command
AssertionError: call scripts\activate.bat && cd "C:\Users\Tommy\Nuitka\tests\distutils\py_modules_only" && python setup.py bdist_nuitka
@kayhayen

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

Can you show the resulting Nuitka command then?

@tommyli3318

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

This is the command passed to subprocess:

['C:\\Users\\Tommy\\Nuitka\\tests\\distutils\\venv_nuitka\\Scripts\\python.exe', '-m', 'nuitka', '--module', '--plugin-enable=pylint-warnings', '--output-dir=C:\\Users\\Tommy\\Nuitka\\tests\\distutils\\py_modules_only\\build\\lib', '--nofollow-import-to=*.tests', '--show-modules', '--remove-output', '--include-module=py_modules_only', 'C:\\Users\\Tommy\\Nuitka\\tests\\distutils\\py_modules_only\\build\\lib\\py_modules_only.py']

@kayhayen

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

The --include-module on the main module is not needed, unlike with packages, which will not automatically be fully included, but only top module, there is no extra benefit from that.

I cannot decipher the error message, it might not be from Nuitka, is it? I guess it is true:

error: [WinError 3] The system cannot find the path specified: 'C:\Users\Tommy\Nuitka\tests\distutils\py_modules_only\build\lib\py_modules_only

However, what is trying to use it?

@kayhayen
Copy link
Member

left a comment

I think I found what's wrong with it.

if self.compile_packages:
self.main_target = self.compile_packages[0]
self.py_modules = False
else:

This comment has been minimized.

Copy link
@kayhayen

kayhayen Aug 14, 2019

Member

Like I said, I am not convinced that not both are allowed to be used at the same time. Can you check what happens if one specifies both?

This comment has been minimized.

Copy link
@tommyli3318

tommyli3318 Aug 15, 2019

Author Member

I've included a new distutils example package_and_module. Nuitka builds it without error with my changed script but ls package_and_module/build/lib/ outputs

example1_package/  libsome_module.a  some_module.pyd*  some_module.pyi

Which seems to be missing the .pyd and .pyi files for the package.

nuitka/distutils/bdist_nuitka.py Outdated Show resolved Hide resolved
os.path.join(
self.build_lib, self.main_package.replace(".", os.path.sep)
)
os.path.join(self.build_lib, self.main_target.replace(".", os.path.sep))

This comment has been minimized.

Copy link
@kayhayen

kayhayen Aug 14, 2019

Member

Ah ok, for packages this is a thing, but not for modules. For those just delete it as a file, and I guess, the .py must be protected from replacing maybe. Trace what do you here, and do the right thing.

But what also strikes me as wrong, is that not all the packages, and not all the module files are deleted, but just the main one. It should be done for all or none I guess. We could really use Nuitka to learn to --replace when it compiles, i.e. delete the input files included. Doing it on the outside is kind of harder to do, but doable, so lets stick to it for now.

For now, this code will probably have issues when multiple packages are included, which is something people do, then I think things might survive. On the other hand, are not Python files all deleted anyway? Can you check if this delete here is even needed?

This comment has been minimized.

Copy link
@tommyli3318

tommyli3318 Aug 14, 2019

Author Member

There are no .py files in the build/lib folder even without this delete for package example_1, that means this is unnecessary, correct?

@@ -217,7 +217,13 @@ def findModule(importing, module_name, parent_package, level, warn):
# We have many branches here, because there are a lot of cases to try.
# pylint: disable=too-many-branches

assert type(module_name) is ModuleName
if type(module_name) is not ModuleName:

This comment has been minimized.

Copy link
@kayhayen

kayhayen Aug 14, 2019

Member

That's definitely not the way to do it. You either obey to its demand, or you convert to ModuleName doing module_name = ModuleName(module_name) but you don't micro manage types like this.

I also think you are calling it with ModuleName in above code. Please just remove this change.

This comment has been minimized.

Copy link
@tommyli3318

tommyli3318 Aug 14, 2019

Author Member

Without this change, I get an AssertionError

Traceback (most recent call last):
  File "C:\Users\Tommy\Nuitka\tests\distutils\venv_nuitka\lib\site-packages\nuitka-0.6.6rc3-py3.7.egg\nuitka\__main__.py", line 184, in <module>
    main()
  File "C:\Users\Tommy\Nuitka\tests\distutils\venv_nuitka\lib\site-packages\nuitka-0.6.6rc3-py3.7.egg\nuitka\__main__.py", line 177, in main
    MainControl.main()
  File "C:\Users\Tommy\Nuitka\tests\distutils\venv_nuitka\lib\site-packages\nuitka-0.6.6rc3-py3.7.egg\nuitka\MainControl.py", line 753, in main
    main_module = createNodeTree(filename=filename)
  File "C:\Users\Tommy\Nuitka\tests\distutils\venv_nuitka\lib\site-packages\nuitka-0.6.6rc3-py3.7.egg\nuitka\MainControl.py", line 130, in createNodeTree
    warn=False,
  File "C:\Users\Tommy\Nuitka\tests\distutils\venv_nuitka\lib\site-packages\nuitka-0.6.6rc3-py3.7.egg\nuitka\importing\Importing.py", line 220, in findModule
    assert type(module_name) is ModuleName
AssertionError

I have no idea why, because as you said, it is called with ModuleName already...

This comment has been minimized.

Copy link
@tommyli3318

tommyli3318 Aug 14, 2019

Author Member

I added debug code to print out type(module_name) before the assertion. The result is <class 'str'>. Somehow ModuleName was not applied correctly to the str? I'm confused :/

@tommyli3318

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

@kayhayen
Made changes and replied to your comments above, please review :). The script now builds distutils/py_modules_only without error, but needs to be tweaked to support the case of both package and module I think

@kayhayen

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

Instead of printing values at crash time, your go to solution should be to consider the callstack and see what it puts there. Your imaginary call stack, your usage of findModule is not the only one. You just encountered a regression of Nuitka develop itself. Please find it and make it a separate PR "fixup_import_module".

Seems we have no coverage in testing for that option until your test.

Yours,
Kay

@kayhayen

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

@JorjMcKie The regression I just noted, should also affect your hinter maybe? Does it not use --include-module command line, or is it purely doing its stuff via plugin calls.

@JorjMcKie

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

The hinted-mods.py user-plugin does this to adjust Nuitka compilation parameters:

  • directly append entries to ´´options.options.plugins_enabled``
  • directly append entries to options.recurse_modules

So it does not (yet) touch options.include_modules or options.include_packages.

@kayhayen

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

Ok, good to know, then it is not affected by the regression, and I need to panic rebase you :)

@JorjMcKie

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

@tommyli3318 tommyli3318 referenced this pull request Aug 15, 2019
2 of 4 tasks complete
@tommyli3318

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

Please find it and make it a separate PR "fixup_import_module".

Ok, it's at #487

@tommyli3318 tommyli3318 force-pushed the tommyli3318:develop-py_modules branch from fb0d938 to 339e5a4 Aug 17, 2019

@tommyli3318 tommyli3318 force-pushed the tommyli3318:develop-py_modules branch from a5aaf6b to e6abd31 Aug 17, 2019

@kayhayen

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

Good job, I really like how this converged to unify the two cases.

@kayhayen kayhayen merged commit 8955ac3 into Nuitka:develop Aug 18, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.