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

Fix errors in manifest file #34

Merged
merged 7 commits into from
Sep 10, 2014
Merged

Conversation

joaofrancese
Copy link
Contributor

I am getting the following error when installing with Pip from PyPI or from source (with -e git+https://...)

reading manifest file 'django_silk.egg-info\SOURCES.txt'
reading manifest template 'MANIFEST.in'
Traceback (most recent call last):
  File "<string>", line 17, in <module>
  File "D:\Dev\AssetTools\git\env\src\silk\setup.py", line 43, in <module>
    'pytz'
  File "C:\Python27\Lib\distutils\core.py", line 152, in setup
    dist.run_commands()
  File "C:\Python27\Lib\distutils\dist.py", line 953, in run_commands
    self.run_command(cmd)
  File "C:\Python27\Lib\distutils\dist.py", line 972, in run_command
    cmd_obj.run()
  File "<string>", line 15, in replacement_run
  File "D:\Dev\AssetTools\git\env\lib\site-packages\setuptools\command\egg_info.py", line 193, in find_sources
    mm.run()
  File "D:\Dev\AssetTools\git\env\lib\site-packages\setuptools\command\egg_info.py", line 279, in run
    self.read_template()
  File "C:\Python27\Lib\distutils\command\sdist.py", line 316, in read_template
    self.filelist.process_template_line(line)
  File "C:\Python27\Lib\distutils\filelist.py", line 118, in process_template_line
    action, patterns, dir, dir_pattern = self._parse_template_line(line)
  File "C:\Python27\Lib\distutils\filelist.py", line 97, in _parse_template_line
    dir = convert_path(words[1])
  File "C:\Python27\Lib\distutils\util.py", line 201, in convert_path
    raise ValueError, "path '%s' cannot end with '/'" % pathname
ValueError: path 'silk/code_generation/' cannot end with '/'
----------------------------------------
Cleaning up...
Command python setup.py egg_info failed with error code 1 in D:\Dev\AssetTools\git\env\src\silk
Storing debug log for failure in C:\Users\joao\pip\pip.log

I was able to install with Pip from source from my fork after this change to the manifest file.

@mtford90
Copy link
Collaborator

Hi,

Weird. Just tried myself and works fine without your changes. What versions of pip + python are you using and what OS?

Here's mine on OSX Mavericks:

mtford@blah ~$ pip3.4 --version
pip 1.5.4 from /Library/Frameworks/Python.framework/Versions/3.4/lib/python3.4/site-packages (python 3.4)
mtford@blah ~$ pip --version
pip 1.4.1 from /Library/Python/2.7/site-packages/pip-1.4.1-py2.7.egg (python 2.7)
mtford@blah ~$ python --version
Python 2.7.5
mtford@blah ~$ python3.4 --version
Python 3.4.0

@joaofrancese
Copy link
Contributor Author

Windows 8.1
ActivePython within a virtualenv

D:\Dev\AssetTools\git>pip --version
pip 1.5.6 from D:\Dev\AssetTools\git\env\lib\site-packages\pip-1.5.6-py2.7.egg (python 2.7)

D:\Dev\AssetTools\git>python --version
Python 2.7.2

mtford90 added a commit that referenced this pull request Sep 10, 2014
Fix errors in manifest file
@mtford90 mtford90 merged commit b46c4e4 into jazzband:master Sep 10, 2014
@mtford90
Copy link
Collaborator

Hey @joaofrancese,

FYI I just had to revert this. All the tests are failing and things have changed too much for me to fix them quickly (and I'm really short on time for the next month or so).

Two major changes have broken the tests, the change from execute_sql to SilkyCursorWrapper and the the use of bulk_create.

I seem to remember that the reason I didn't use bulk_create was because we needed to have an primary key of the created models so that they could use them in the relationship. And bulk_create didn't do this. I see that you've used set_primary_key. Is this new? Worried about backwards compatability to django 1.5/1.6 as I see it mentioned in this Django ticket: https://code.djangoproject.com/ticket/19527 but that ticket hasn't been resolved...

If you're willing to incorporate your changes into the unit tests and travis build I can give you commit rights to repository so that you don't have to wait for me to approve. If set_primary_key is not backwards compatible will also need to add some code that follows the old method of saving each individually.

Also, if you look at the dev branch I also quickly implemented executemany of the cursor wrapper. It's not entirely correct as each execution of the sql statement will have the same start + end time but I imagine will do for now. See the inline comments for my suggestions on how we could improve this...

Cheers!

@joaofrancese
Copy link
Contributor Author

I implemented set_primary_keys in bulk_create() in my fork of django-mssql, using an SQL Server-specific solution, because the inability to use bulk_create in such situations was severely hampering my project's speed. That's why I didn't make a pull request of my changes. Though, the way I used it in silk should be backwards-compatible, because I check for a flag I created in DatabaseFeatures, and fall back to saving each object individually in case the flag doesn't exist.

It's just a guess, but the test may be failing because I moved the updating of request.num_sql_queries from save() / bulk_create() to DataCollector.finalise(). The previous implementation in bulk_create wasn't working, because it was updating the Request object directly in the database using update() with F expressions, but the in-memory request object was saved afterwards, negating these changes. I'd have to fix it anyway, thought it would just be easier to have the code in only one place, so I put it in finalise() and removed the manager. It would create inconsistencies if SQLQuery objects are saved outside of this method, but I looked around it doesn't seem to be the case. Do you think it's okay to leave it this way?

I'll review the tests to fix either the code or the tests.

@mtford90
Copy link
Collaborator

Ah right understood! It seems that when you've made a pull request and then commit further changes after the fact, github pushes those commits into the pull request too! So when I merged this in I got everything, not just the manifest changes... Not sure why that's the behaviour they chose.

Wrt. the flag checking, cool, somehow I missed that addition, if the tests pass and the travis build passes then we can know for sure that the backwards compatibility works.

Wrt.request.num_sql_queries, yep I think that's a better solution anyway. The placement in bulk_create was more a result of my experimenting... So yeah I think it's okay to leave it that way.

Awesome, much appreciate that.

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.

2 participants