> On Aug. 10, 2016, 12:02 p.m., Kevin Klues wrote:
> > support/mesos-style.py, lines 228-234
> > <https://reviews.apache.org/r/50910/diff/1/?file=1467414#file1467414line228>
> >
> > I know we didn't talk about this, but I realized recently that we
> > actually *have* to run pylint inside the virtual environment, otherwise it
> > runs using the system python, which is not what we want..... Especially for
> > import libraries.
>
> Haris Choudhary wrote:
> There are two approaches to this:
>
> 1) We can either activate the virtualenv from the CLI and than run
> pylint. But that means if the virtualenv is not created within the CLI we
> will have to create it and activate it.
>
> 2) We can integrate the virtualenv to the project wide bootstrap and thus
> ensuring the the virtualenv is created for the project on bootstrapping
> mesos. This seems to be the better way to do it however might require
> significant changes as opposed to (1). It'd be desirable to have a
> project-wide virtualenv at some point however even if we choose to not do so
> right now. A thing to note here is that if we integrate it to the project
> wide bootstrap, we'll need users to have virtualenv installed for mesos
> instead of only the CLI.
>
> Kevin Klues wrote:
> I'd prefer #2, but it's a bigger change and has the downside that the
> virtualenv dependency will become immediate if we add it to the top-level
> bootstrap. Curious what @vinodkone thinks.
>
> Kevin Klues wrote:
> I think this should suffice for now (including fixes to the indentation):
> ```
> cli_dir = os.path.abspath(self.source_dirs[0])
> source_files = ' '.join(source_paths)
>
> p = subprocess.Popen(
> ['source {virtualenv}/bin/activate; \
> PYTHONPATH={lib_dir}:{bin_dir} pylint --rcfile={config}
> --ignore={ignore} {files}'.\
> format(virtualenv=os.path.join(cli_dir, '.virtualenv'),
> lib_dir=os.path.join(cli_dir, 'lib'),
> bin_dir=os.path.join(cli_dir, 'bin'),
> config=os.path.join(cli_dir, 'pylint.config'),
> ignore=os.path.join(cli_dir, 'bin', 'mesos'),
> files=source_files)],
> shell=True, stdout=subprocess.PIPE)
> ```
>
> Kevin Klues wrote:
> Though,a ctually, this should come in a subsequent commit. Just fix the
> indenting and the other comments from Vinod and I'll add this in as part of
> my commit that actually populates the `cli_dir`.
I would prefer something like this:
```
p = subprocess.Popen([
'pylint',
'--rcfile=%s' % os.path.join(cli_dir, 'pylint.config'),
'--ignore=%s' % os.path.join(cli_dir, 'bin', 'mesos'),
] + source_paths,
env={ "PYTHONPATH" : "%s:%s" % (lib_path, bin_path)},
stdout=subprocess.PIPE)
```
No `shell=True`. As for how to activate a virtualenv... I'm not sure. But it
should still be possible without using `shell=True`, I hope.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50910/#review145386
-----------------------------------------------------------
On Aug. 10, 2016, 3:43 p.m., Haris Choudhary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50910/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2016, 3:43 p.m.)
>
>
> Review request for mesos, Kevin Klues and Vinod Kone.
>
>
> Bugs: Mesos-6006
> https://issues.apache.org/jira/browse/Mesos-6006
>
>
> Repository: mesos
>
>
> Description
> -------
>
> It currently doesn't run over any files in the code base, but we will
> be adding the new python CLI in a subsequent commit, which will use
> this new linter.
>
>
> Diffs
> -----
>
> support/mesos-style.py 061cbe711bd9407b7341fa146f8c723eaa9fc438
>
> Diff: https://reviews.apache.org/r/50910/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Haris Choudhary
>
>
