mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Haris Choudhary <haris0...@gmail.com>
Subject Re: Review Request 50910: Added a python linter to mesos-style.cpp.
Date Thu, 11 Aug 2016 20:02:10 GMT


> On Aug. 10, 2016, 7: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`.
> 
> Joseph Wu wrote:
>     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.

We'll have to keep shell=True as activating a virtualenv is not possible without it.


- Haris


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50910/#review145386
-----------------------------------------------------------


On Aug. 10, 2016, 10: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, 10: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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message