Skip to content

Conversation

@igormironchik
Copy link
Contributor

Now it's simple to initialize and run mutate_cpp in the repository, just clone and do the job. Patches made to work on Ubuntu 16.04. But patches now use \r\n line endings, this is default behavior for git?!

@nlohmann
Copy link
Owner

Thanks for the PR. I shall have a look at it soon. I hope that the project was still useful.

@nlohmann nlohmann self-requested a review November 27, 2017 11:39
@igormironchik
Copy link
Contributor Author

mutate_cpp is really interesting but usual C++ file mutates approximately 700 times. This is 700 rebuilds for one file (if code places in header). In a small project on usual hardware this is a working day. That is why I see mutate_cpp as a toy for developers... But it can help... If a developer has enough time or a good hardware.

README.md Outdated
```bash
virtualenv -p python3 venv
venv/bin/pip install -r requirements
python3 -m venv venv
Copy link
Owner

@nlohmann nlohmann Nov 28, 2017

Choose a reason for hiding this comment

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

This assumes that the module venv is installed, right? I tried this on a Ubuntu system today, and virtualenv worked, whereas python -m venv did not.

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 have opposite situation. virtualenv -p python3 venv doesn't work for me on Ubuntu 16.04 whereas python3 -m venv venv is working. And on page https://docs.python.org/3/tutorial/venv.html you can see example:

"To create a virtual environment, decide upon a directory where you want to place it, and run the venv module as a script with the directory path"

python3 -m venv tutorial-env

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, then there is apparently more than 1 way to do this...

Copy link
Owner

Choose a reason for hiding this comment

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

Ok. Please leave this as it is. Maybe add a comment below that some systems require python3 -m venv venv to create a virtual environment.

import os.path
import sys

sys.path.append( os.path.dirname( os.path.abspath( __file__ ) ) )
Copy link
Owner

Choose a reason for hiding this comment

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

I never needed that so far. Is this to make the code robust against the working directory?

Copy link
Contributor Author

@igormironchik igormironchik Nov 29, 2017

Choose a reason for hiding this comment

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

Yes, this line is for create_db.py to be working when launched exactly from the root directory of the repository. I had problems with from config import ... Python couldn't find module config. And when I added this line to app/__init__.py create_db.py became working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, user can tune python's environment so all will work without this line. But I'm not sure about from config import ...., why not from app.config import ....?


# lines: context before
patch_lines += [' ' + x + '\n' for x in context_before]
patch_lines += [' ' + x + "\r\n" for x in context_before]
Copy link
Owner

Choose a reason for hiding this comment

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

Why is \r\n needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Git can clone files with different line endings mode. By default, as I guess, git clone with \r\n and pushes with \n. So on my Ubuntu 16.04 I have repositories with \r\n line endings in files. So patch didn't work with \n and works with \r\n. But to be fair it's better to handle both situations...

README.md Outdated
```bash
virtualenv -p python3 venv
venv/bin/pip install -r requirements
python3 -m venv venv
Copy link
Owner

Choose a reason for hiding this comment

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

Ok. Please leave this as it is. Maybe add a comment below that some systems require python3 -m venv venv to create a virtual environment.


# step 4: revert patch
self.__execute_command_timeout('patch -p1 --reverse --input={patchfile} {inputfile}'.format(patchfile=patchfile.name, inputfile=file.filename),
cwd='/')
Copy link
Owner

Choose a reason for hiding this comment

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

What is the rationale for this changed block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed to work properly on Ubuntu 16.04. If launch patch -p1 --reverse --input={patchfile} then patch will fail and request file name to patch. And with patch -p1 --reverse --input={patchfile} {inputfile} all works as expected.

from config import SQLALCHEMY_MIGRATE_REPO
from app.config import SQLALCHEMY_DATABASE_URI
from app.config import SQLALCHEMY_MIGRATE_REPO
from app import db
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't these changes also be required in db_migrate.py and db_upgrade.py?

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 think so. I didn't use these scripts, so didn't make any changes to them.

@igormironchik
Copy link
Contributor Author

I restored README.md and replaced from config import ... with from app.config import ... in db scripts. And I guess that PR #2 is correct because venv/bin/pip install -r requirements seems to need -r requirements.txt

@nlohmann nlohmann merged commit 1335e0a into nlohmann:master Nov 30, 2017
@nlohmann
Copy link
Owner

Thanks a lot!

@Stratosz
Copy link

Hello, just wanted to let you guys know that this PR breaks something on OS X (10.11). I'm not looking for a fix, I just needed to use the library for a university project, and I'm not familiar with Python to fix it.

Everything works fine, except for the queue start action. I managed to get the project working by reverting to the commit before this PR was approved.

127.0.0.1 - - [11/Apr/2018 17:10:35] "GET /queue/start HTTP/1.1" 302 -
Exception in thread Thread-2:
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/threading.py", line 916, in _bootstrap_inner
    self.run()
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/threading.py", line 864, in run
    self._target(*self._args, **self._kwargs)
  File "/Users/***/mutate_cpp/app/utils/Executor.py", line 42, in main
    self.workflow(patch)
  File "/Users/***/mutate_cpp/app/utils/Executor.py", line 89, in workflow
    self.__execute_command_timeout('patch -p1 --input={patchfile} {inputfile}'.format(patchfile=patchfile.name, inputfile=file.filename), cwd='/')
  File "/Users/***/app/utils/Executor.py", line 71, in __execute_command_timeout
    raise subprocess.CalledProcessError(errcode, command, stdout)
subprocess.CalledProcessError: Command 'patch -p1 --input=/var/folders/sh/026s5d0s7p32_ytprmw0lyx00000gn/T/tmpwcvbyykd /Users/***/cmake-example/src/example.cpp' returned non-zero exit status 1.

@igormironchik
Copy link
Contributor Author

Could you run patch -p1 --input=/var/folders/sh/026s5d0s7p32_ytprmw0lyx00000gn/T/tmpwcvbyykd /Users/***/cmake-example/src/example.cpp by hands? What does it say? I guess that this is problem of line endings in patch. I guess that this is impossible to generate a patch in Python that will handle all possible situations because in Python line endings always converts to \n. I thought about it and didn't find a way to solve the problem of line endings in Python when reading text.

@igormironchik
Copy link
Contributor Author

I caught exactly the same problem on my Ubuntu on that days, that is why I created a PR that solved the problem for me. You can edit mutate_cpp/app/utils/SourceFile.py

And change \r\n to \n

# lines: context before
patch_lines += [' ' + x + "\r\n" for x in context_before]
# line: the old value
patch_lines.append('-' + old_line + "\r\n")
# line: the new value
if replacement.new_val is not None:
    patch_lines.append('+' + new_line + "\r\n")
# lines: context after
patch_lines += [' ' + x + "\r\n" for x in context_after]

@igormironchik
Copy link
Contributor Author

I can suggest to refuse from patch at all, and implement patching and reverting in Python, but again line endings can be messed...

@alirezah7 alirezah7 mentioned this pull request Jun 1, 2018
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.

3 participants