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

Improvements related to bdist_msi --target_name #1648

Merged
merged 5 commits into from Oct 18, 2022

Conversation

cainesi
Copy link
Contributor

@cainesi cainesi commented Oct 16, 2022

Improves documentation for the target_name parameter and adds a new test.

@cainesi
Copy link
Contributor Author

cainesi commented Oct 16, 2022

The new test works when run on its own, but fails when all the tests are run together ( error: "SystemExit: error: could not create 'build\lib\hello.py': No such file or directory"). I'm not familiar another with pytest to be sure what's going on. Any ideas?

@marcelotduarte
Copy link
Owner

marcelotduarte commented Oct 16, 2022

The error is relative to sandbox. run_setup is based on the run_setup from setuptools.
Either there's a bug in my implementation or it's really recording outside the sandbox.

@cainesi
Copy link
Contributor Author

cainesi commented Oct 16, 2022

Yes, I think you are right. Even if I just create a duplicate of the existing test_bdist_msi() with a different name, it still fails.

It seems like it is a problem with doing two freezes sequentially within a single python instance-- just checked it, and it also popped up in my build setup. I recall running across a similar problem several years ago. That problem turned out to be due to some information being stored globally in the msilib module, and that needed to be reset between freezes (the fix was in PR #419). Maybe some similar problem in the testing code (took a quick look and didn't see it though).

@marcelotduarte
Copy link
Owner

If you put your test as the first test, then it passes and the other fails?

@cainesi
Copy link
Contributor Author

cainesi commented Oct 16, 2022

If you put your test as the first test, then it passes and the other fails?

Even if I just make an identical copy of the test_bdist_msi() test (except for a different name) it fails when I try to run all tests.

@marcelotduarte marcelotduarte merged commit ee6df3d into marcelotduarte:main Oct 18, 2022
@marcelotduarte
Copy link
Owner

I fixed run_setup.
Thanks!

@cainesi
Copy link
Contributor Author

cainesi commented Oct 20, 2022

Thanks @marcelotduarte, that did fix it for my tests.

Did you figure out what the underlying problem was? Was it something about msilib again?

I am encountering a similar problem (with two invocations of setuptools.distutils.core.run_setup) and am trying to figure out if I can fix it. I took a look at your changes but was not exactly sure how they worked.

@marcelotduarte
Copy link
Owner

Did you figure out what the underlying problem was? Was it something about msilib again?

Apparently yes. I started from that assumption, as you suggested it could be a problem with msilib.

I am encountering a similar problem (with two invocations of setuptools.distutils.core.run_setup) and am trying to figure out if I can fix it. I took a look at your changes but was not exactly sure how they worked.

Previously I did a very simplified run_setup (#1484), based on setuptools, but I didn't remove the already loaded setuptools modules, as they did. I figured I would need to remove cx_Freeze as well, but as my initial tests worked for bdist_msi and bdist_rpm, I left it at that.
Now in #1649 I removed cx_Freeze and also msilib. I chose to just make a monkey patch so I don't have to duplicate everything they did, just duplicate the run_setup itself.

Instead of using setuptools.distutils.core.run_setup you can use setuptools.sandbox.run_setup, but you can also use cx_Freeze.sandbox.run_setup.

@cainesi
Copy link
Contributor Author

cainesi commented Oct 20, 2022

Thanks @marcelotduarte. In my case I'm using distutils.core.run_setup. I'll check if hiding some of the scripts temporarily also fixes my problem, similar to what is done with the sandbox'ed version with your monkey patch.

I confess I am still not sure exactly why the change you made to sandbox fixes things. It seems like this problem must be different from the old msilib problem (since (i) the fix for the old problem is still in the cx_freeze code; and (ii) the old problem didn't prevent the freeze from completing, it just caused the resulting MSI file to be broken). Will try to take a look when I get some free time.

@cainesi cainesi deleted the bdist_msi_doc_improvement branch October 20, 2022 13:19
@cainesi
Copy link
Contributor Author

cainesi commented Oct 28, 2022

FYI, I figured out the underlying problem. It is global data again. This time in setuptools_distutils\dir_util.py. In particular the function mkpath() which is called by copy_tree(). mkpath() keeps a global cache of all the directories that have been created, so it does not try to re-create them. Obviously that goes wrong if you (i) do a freeze, (ii) delete the directories, (iii) try to re-do the freeze in the same python session.

I expectt we can fix this by forcing a reload of setuptools_distutils\dir_util.py (like we do for msilib), or by reaching into dir_util.py to reset _path_created.

@marcelotduarte
Copy link
Owner

marcelotduarte commented Oct 28, 2022

I think you can do like setuptools do. In https://github.com/pypa/setuptools/blob/main/setuptools/sandbox.py, hidesetuptools calls _clear_module and remove the _distutils_hack. When I used a monkey patch over it, I include two modules to it.
Maybe you can run with hidesetuptools(), then import the desired run_setup function and run it.

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.

None yet

2 participants