-
Notifications
You must be signed in to change notification settings - Fork 53
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
cEP-0017.md: cEP describing design of cib #117
Conversation
cEP-0017.md
Outdated
|
||
# Abstract | ||
|
||
This cEP describes the design of the cib tool which will allow it to install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line contains following spacing inconsistencies:
- Trailing whitespaces.
Origin: SpaceConsistencyBear, Section: spacing
.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpobuqqos9/cEP-0017.md
+++ b/tmp/tmpobuqqos9/cEP-0017.md
@@ -12,7 +12,7 @@
# Abstract
-This cEP describes the design of the cib tool which will allow it to install
+This cEP describes the design of the cib tool which will allow it to install
individual bears and their dependencies easily.
The cEP also suggests changes to Requirement classes and using conda packages
cEP-0017.md
Outdated
# Abstract | ||
|
||
This cEP describes the design of the cib tool which will allow it to install | ||
individual bears and their dependencies easily. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line contains following spacing inconsistencies:
- Trailing whitespaces.
Origin: SpaceConsistencyBear, Section: spacing
.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpobuqqos9/cEP-0017.md
+++ b/tmp/tmpobuqqos9/cEP-0017.md
@@ -13,7 +13,7 @@
# Abstract
This cEP describes the design of the cib tool which will allow it to install
-individual bears and their dependencies easily.
+individual bears and their dependencies easily.
The cEP also suggests changes to Requirement classes and using conda packages
for bears such as DartLintBear.
cEP-0017.md
Outdated
The current design of the cib tool allows it to perform basic operations such as | ||
running pip install on bears and then checking their dependencies. However, the | ||
design of the cib tool needs to be able to handle other special cases such as of | ||
bears that have different installation instructions for each distribution such as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line contains following spacing inconsistencies:
- Trailing whitespaces.
Origin: SpaceConsistencyBear, Section: spacing
.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpobuqqos9/cEP-0017.md
+++ b/tmp/tmpobuqqos9/cEP-0017.md
@@ -24,7 +24,7 @@
The current design of the cib tool allows it to perform basic operations such as
running pip install on bears and then checking their dependencies. However, the
design of the cib tool needs to be able to handle other special cases such as of
-bears that have different installation instructions for each distribution such as
+bears that have different installation instructions for each distribution such as
DartLintBear and bears that have bear dependencies.
Additionally, the tool should also be able to provide verbose output, ask user for
cEP-0017.md
Outdated
running pip install on bears and then checking their dependencies. However, the | ||
design of the cib tool needs to be able to handle other special cases such as of | ||
bears that have different installation instructions for each distribution such as | ||
DartLintBear and bears that have bear dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line contains following spacing inconsistencies:
- Trailing whitespaces.
Origin: SpaceConsistencyBear, Section: spacing
.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpobuqqos9/cEP-0017.md
+++ b/tmp/tmpobuqqos9/cEP-0017.md
@@ -25,7 +25,7 @@
running pip install on bears and then checking their dependencies. However, the
design of the cib tool needs to be able to handle other special cases such as of
bears that have different installation instructions for each distribution such as
-DartLintBear and bears that have bear dependencies.
+DartLintBear and bears that have bear dependencies.
Additionally, the tool should also be able to provide verbose output, ask user for
permission before proceeding and install bears related to a specific language.
cEP-0017.md
Outdated
bears that have different installation instructions for each distribution such as | ||
DartLintBear and bears that have bear dependencies. | ||
|
||
Additionally, the tool should also be able to provide verbose output, ask user for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line contains following spacing inconsistencies:
- Trailing whitespaces.
Origin: SpaceConsistencyBear, Section: spacing
.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpobuqqos9/cEP-0017.md
+++ b/tmp/tmpobuqqos9/cEP-0017.md
@@ -27,7 +27,7 @@
bears that have different installation instructions for each distribution such as
DartLintBear and bears that have bear dependencies.
-Additionally, the tool should also be able to provide verbose output, ask user for
+Additionally, the tool should also be able to provide verbose output, ask user for
permission before proceeding and install bears related to a specific language.
cEP-0017.md
Outdated
|
||
Here is the procedure for the installation of the bears: | ||
|
||
1. Identify the valid bear names that need to be installed as specified by the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line contains following spacing inconsistencies:
- Trailing whitespaces.
Origin: SpaceConsistencyBear, Section: spacing
.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpobuqqos9/cEP-0017.md
+++ b/tmp/tmpobuqqos9/cEP-0017.md
@@ -35,7 +35,7 @@
Here is the procedure for the installation of the bears:
-1. Identify the valid bear names that need to be installed as specified by the
+1. Identify the valid bear names that need to be installed as specified by the
user on the command line.
2. Install bear on the valid bear names. We will run commands from CondaRequirement
to check whether the package is uploaded to anaconda.org and then check whether
cEP-0017.md
Outdated
2. Install bear on the valid bear names. We will run commands from CondaRequirement | ||
to check whether the package is uploaded to anaconda.org and then check whether | ||
it can be installed from PyPi using PipRequirement. | ||
* This will require to create conda packages for bears that have different |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line contains following spacing inconsistencies:
- Trailing whitespaces.
Origin: SpaceConsistencyBear, Section: spacing
.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpobuqqos9/cEP-0017.md
+++ b/tmp/tmpobuqqos9/cEP-0017.md
@@ -40,7 +40,7 @@
2. Install bear on the valid bear names. We will run commands from CondaRequirement
to check whether the package is uploaded to anaconda.org and then check whether
it can be installed from PyPi using PipRequirement.
- * This will require to create conda packages for bears that have different
+ * This will require to create conda packages for bears that have different
installation instructions for each distribution. This is done by making `bld.bat`
file that has installation instruction for Windows and similarly a `bld.sh` file
that has installation instruction for Linux.
cEP-0017.md
Outdated
installation instructions for each distribution. This is done by making `bld.bat` | ||
file that has installation instruction for Windows and similarly a `bld.sh` file | ||
that has installation instruction for Linux. | ||
3. After installing the bear we proceed to installing its requirements. We call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line contains following spacing inconsistencies:
- Trailing whitespaces.
Origin: SpaceConsistencyBear, Section: spacing
.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpobuqqos9/cEP-0017.md
+++ b/tmp/tmpobuqqos9/cEP-0017.md
@@ -44,7 +44,7 @@
installation instructions for each distribution. This is done by making `bld.bat`
file that has installation instruction for Windows and similarly a `bld.sh` file
that has installation instruction for Linux.
-3. After installing the bear we proceed to installing its requirements. We call
+3. After installing the bear we proceed to installing its requirements. We call
methods from the requirement classes to do this.
* Add new Requirement classes to support different dependencies from various linters.
* The DistributionRequirement class needs to be split into different classes for each
cEP-0017.md
Outdated
3. After installing the bear we proceed to installing its requirements. We call | ||
methods from the requirement classes to do this. | ||
* Add new Requirement classes to support different dependencies from various linters. | ||
* The DistributionRequirement class needs to be split into different classes for each |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line contains following spacing inconsistencies:
- Trailing whitespaces.
Origin: SpaceConsistencyBear, Section: spacing
.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpobuqqos9/cEP-0017.md
+++ b/tmp/tmpobuqqos9/cEP-0017.md
@@ -47,7 +47,7 @@
3. After installing the bear we proceed to installing its requirements. We call
methods from the requirement classes to do this.
* Add new Requirement classes to support different dependencies from various linters.
- * The DistributionRequirement class needs to be split into different classes for each
+ * The DistributionRequirement class needs to be split into different classes for each
package manager and call its method to introduct more consistency.
4. We then proceed to install bear dependencies such as for ClangCloneDetectionBear by
running `install_bears` method on each dependency bear.
cEP-0017.md
Outdated
* Add new Requirement classes to support different dependencies from various linters. | ||
* The DistributionRequirement class needs to be split into different classes for each | ||
package manager and call its method to introduct more consistency. | ||
4. We then proceed to install bear dependencies such as for ClangCloneDetectionBear by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line contains following spacing inconsistencies:
- Trailing whitespaces.
Origin: SpaceConsistencyBear, Section: spacing
.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpobuqqos9/cEP-0017.md
+++ b/tmp/tmpobuqqos9/cEP-0017.md
@@ -49,7 +49,7 @@
* Add new Requirement classes to support different dependencies from various linters.
* The DistributionRequirement class needs to be split into different classes for each
package manager and call its method to introduct more consistency.
-4. We then proceed to install bear dependencies such as for ClangCloneDetectionBear by
+4. We then proceed to install bear dependencies such as for ClangCloneDetectionBear by
running `install_bears` method on each dependency bear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closes #10 pls
cEP-0017.md
Outdated
This cEP describes the design of the cib tool which will allow it to install | ||
individual bears and their dependencies easily. | ||
|
||
The cEP also suggests changes to Requirement classes and using conda packages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repeated spaces.
Also Requirement
should be in backticks. Lots of backticks needed throughout this cEP
cEP-0017.md
Outdated
Here is the procedure for the installation of the bears: | ||
|
||
1. Identify the valid bear names that need to be installed as specified by the | ||
user on the command line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indent.
please run markdownbear / remark on your .md.
I would dearly love to see https://github.com/coala/coala-bears/blob/master/bears/generate_package.py moved from coala-bears to a different location. it could be coala, cib, dependency_management. I really dont care. Ah, coala/coala-bears#1199 is the related issue in that repo. And PEP 420 (coala/coala#3545) is why I want it removed .. ;-) Also I would like to see some evaluation of https://github.com/mbodenhamer/depman (who is a co-mentor on this project) to see if some of its functionality may help this project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please specify the user interface.
And indicate scenarios of usage which will work.
Note any which will not work, because they cant, or because they are too complex and are out of scope for this project.
What are the minimum requirements for it to work?
Will cib
work on Alpine Linux with only python, coala
and cib
installed? Or does coala-bears
need to be installed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useful feature, and an add-on for cib
coala/coala#5456
Before performing review itself I would like to raise several questions/topics; if they're out of the scope please just skip it; if it could be a critical open question - I suggest to mention it in the doc:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address my questions above (we can also discuss it in gitter, but then duplicate answers there).
We could get the password using If more than 1 package managers are available to install the linter on the platform, I think we could simply chose any 1 and continue. But if we should be giving a higher priority to any one such as For conflicting dependencies, print a warning to the user regarding the conflict or simply putting out |
@Udayan12167 please review |
cEP-0017.md
Outdated
1. Create a new repository called `coala-bear packages`. | ||
2. This repository will have a folder called `conda packages` that contains conda packages for | ||
all the bears that require different installation instructions on each distribution. Each | ||
conda package requires 3 files i.e. `meta.yaml`, `build.sh`, `bld.bat`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above (line 90) you have mentioned bld.sh
and here it is build.sh
. You should stick to one name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(fixed)
cEP-0017.md
Outdated
|
||
### Splitting `DistributionRequirement` | ||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use syntax highlighting for Python using ```python
cEP-0017.md
Outdated
|
||
### Package Requirement | ||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Syntax can be highlighted using ```yaml
e.g
- pip:
name: package_name
version: 0.11
cEP-0017.md
Outdated
|
||
1. Identify the valid bear names that need to be installed as specified by the | ||
user on the command line. | ||
2. Install bear on the valid bear names. We will run commands from `CondaRequirement` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove We
and other pronouns. Here "we" refers to "cib"?
cEP-0017.md
Outdated
|
||
## Introduction | ||
|
||
The current design of the cib tool allows it to perform basic operations such as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consistent backticks for cib
pls
cEP-0017.md
Outdated
|
||
Another alternative to `cib` that can be used for installing bears and their dependencies | ||
is Ansible. Ansible allows to orchestrate steps of any process | ||
for different distributions by creating `playbooks` in YAML. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
playbooks isnt a code literal -- it is a English term created by Ansible, so should be in double quotes "playbooks"
cEP-0017.md
Outdated
|
||
Ansible provides numerous | ||
[packaging modules](http://docs.Ansible.com/Ansible/latest/modules/list_of_packaging_modules.html) | ||
that can be used to install dependencies of `Bears`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> "Bears" - terminology coala created
cEP-0017.md
Outdated
1. Identify the valid bear names that need to be installed as specified by the | ||
user on the command line. | ||
2. Install bear on the valid bear names. We will run commands from `CondaRequirement` | ||
to check whether the package is uploaded to `Anaconda.org` and then check whether |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anaconda.org should be a link.
Why only Anaconda.org ? That is only one group of channels/respositories.
There are others, like Bioconda, which are more open.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bioconda channel is a Conda channel providing bioinformatics related packages for Linux and Mac OS.
We don't want to use the bioconda channel. I will investigate more on other channels or creating a new channel.
cEP-0017.md
Outdated
|
||
choice = raw_input().lower() | ||
if choice in consts.TRUE_STRINGS: | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consecutive spaces .. eww
cEP-0017.md
Outdated
|
||
REQUIREMENTS = {} | ||
|
||
def __init__(self, platform: str, package: str, version="", repo=""): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single quotes for string literals (everywhere).
cEP-0017.md
Outdated
raise NotImplementedError | ||
|
||
@lru_cache() | ||
def get_platform(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be a class method, otherwise each subclass will have its own lru_cache.
cEP-0017.md
Outdated
version: 0.11 | ||
``` | ||
|
||
This can be used to install the `pip` dependencies for `Bears`. Ansible currently |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> "Bears"
cEP-0017.md
Outdated
`GNUIndentBear` that uses a `DistributionRequirement` as shown: | ||
|
||
```python | ||
REQUIREMENTS = {DistributionRequirement('indent')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need an example which needs a specific version.
ArtisticStyleBear
is a better example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ArtisticStyleBear
doesn't mention a version
cEP-0017.md
Outdated
return True | ||
elif choice in consts.FALSE_STRINGS: | ||
return False | ||
else : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra space.
please run your sample code through pyflakes and pycodestyle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cEP-0017.md
Outdated
|
||
- name: Install bear_name | ||
package: > | ||
name={{ bear_name }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bear_name
isnt a good variable name for a dependency.
{{<program_name>}}
would be more intuitive.
e.g.
name={{yamllint}}
Also, bears can have multiple dependencies, so any 1:1 relationship in names should be avaoided.
and if we ever add the second yamllint (ruby iirc) in a second bear, then we can simply use a different name, without it looking too weird.
name={{ruby_yamllint}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes that is a mistake
it should have been {{ package_name }}
cEP-0017.md
Outdated
|
||
@classmethod | ||
@lru_cache() | ||
def get_platform(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a class method must take an argument cls
.
cEP-0017.md
Outdated
def get_platform(): | ||
return platform.system(), distro.id() | ||
|
||
def check_platform(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arg self missing
def install_bears(bear_names_list): | ||
bear_failed_list = [] | ||
for bear_name in bear_names_list: | ||
if PipRequirement(bear_name).is_installed() or \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the real code, please avoid \
.
print(bear_name + ' is already installed.') | ||
continue | ||
if not PipRequirement(bear_name).install_package() or \ | ||
not CondaRequirement(bear_name).install_package(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this indentation is not OK for real code.
see PEP8 for explanation
""" | ||
raise NotImplementedError | ||
|
||
@classmethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you implement this, it should probably be a static method, or even a function outside of the class.
REQUIREMENTS = { | ||
DistributionRequirement( | ||
apt_get='astyle', | ||
dnf='astyle' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing ,
ack ca27c58 |
@gitmate-bot ff |
Hey! I'm GitMate.io! This pull request is being fastforwarded automatically. Please DO NOT push while fastforward is in progress or your changes would be lost permanently |
Automated fastforward with GitMate.io was successful! 🎉 |
Ubuntu, CentOS, Fedora, Arch Linux, openSUSE/SUSE, | ||
Gentoo Linux, Darwin | ||
:param package: A string with the name of the package to be installed. | ||
:param version: Version string. Leave empty to specify latest version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to recommend that tested
version could be specified here as well; so, the end user may install the latest
version or the latest version which was tested
or any custom
version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, it's too late as far as PR is merged, just FYI @jayvdb @anctartica
Closes #10