-
Notifications
You must be signed in to change notification settings - Fork 28
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
implemented alternative versioning mechanism to __conda_version__ (issue #53) #58
Conversation
rdkit-postgresql/meta.yaml
Outdated
git_rev: Release_2017_03 | ||
#git_url: https://github.com/rdkit/rdkit.git | ||
#git_rev: Release_2017_03 | ||
git_url: https://github.com/ptosco/rdkit.git |
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 minor thing, but this change looks clearly unintentional
rdkit/meta.yaml
Outdated
|
||
source: | ||
git_url: https://github.com/rdkit/rdkit.git | ||
git_rev: Release_2017_03 | ||
git_url: https://github.com/ptosco/rdkit.git |
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.
same here, of course.
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.
Hi Riccardo, I have left those paths in to enable testing while the changes in the rdkit trunk are not yet merged into master - I'll revert to the usual paths once the rdkit PR is accepted.
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.
@ptosco @rvianello For various reasons I prefer the versioning method seen in #59, mainly because the versioning is automatic. If you don't have any complaints, I could do a PR against this one.
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 don't mind, I made this PR just to restore a working conda build while keeping the same versioning tag as with __conda_version__
, but of course I'm happy with any versioning mechanism.
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 do not have any strong preferences, I like the general approach in #59 because it tends to reduce the amount of code we may need to maintain, I wasn't sure if it could work with how tags are formatted or positioned on the various branches in the rdkit repository. This PR on the other hand also looked fine to me as it didn't introduce any change apart the fix.
The other PR is now merged. |
OK, I have changed |
boost/bld.bat
Outdated
@@ -1,3 +1,7 @@ | |||
:: Until Anaconda does not properly detect VS2017 when |
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.
Do you really mean "Until Anaconda properly detects" ?
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, sorry - Italians negate that kind of sentences. And do all sorts of other evil things, but let's not talk about that now.
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 have fixed this.
eigen/check_VS2017.bat
Outdated
@@ -0,0 +1,46 @@ | |||
::check_VS2017 |
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 guess there's no way we can avoid the duplication of this file everywhere is there?
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.
That's really crap, isn't it? We may create a build_support
directory in conda-rdkit
and put it there, then source it from there:
call "%RECIPE_DIR%\..\build_support\check_VS2017.bat"
I have tested it right now and it works. Shall I commit this change?
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 am guilty of some other duplicated scripts in the rdkit and cartridge recipes, but I would favor the idea of having a single copy in a sibling directory
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 have removed duplication.
I can fix the conflicts pre-merge. no problem there. |
@bp-kelley the |
is it ok if I let you guys figure this out and just wait for you to tell me what the answer is? |
This change, coupled with the changes in the rdkit master branch, allows building the rdkit and rdkit-postgresql on newer conda version where conda_version has been obsoleted, and fixes issue #53.