Skip to content
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

Closed
wants to merge 0 commits into from

Conversation

ptosco
Copy link
Contributor

@ptosco ptosco commented Nov 2, 2017

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.

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
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, of course.

Copy link
Contributor Author

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.

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.

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 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.

Copy link
Contributor

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.

@greglandrum
Copy link
Member

The other PR is now merged.

@ptosco
Copy link
Contributor Author

ptosco commented Nov 7, 2017

OK, I have changed ptosco into rdkit. However I left in master instead of Release_2017_09 or the build will fail due to missing pkg_version.py. This is why git is whinging about a conflict.

boost/bld.bat Outdated
@@ -1,3 +1,7 @@
:: Until Anaconda does not properly detect VS2017 when
Copy link
Member

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" ?

Copy link
Contributor Author

@ptosco ptosco Nov 7, 2017

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.

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 fixed this.

@@ -0,0 +1,46 @@
::check_VS2017
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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

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 removed duplication.

@greglandrum
Copy link
Member

I can fix the conflicts pre-merge. no problem there.

@bp-kelley
Copy link

@ptosco , could you see if #53 helps on windows with conda 3 versioning? I'm unclear if this is related to what you are trying to do with windows

@ptosco
Copy link
Contributor Author

ptosco commented Nov 7, 2017

@bp-kelley the check_VS2017.bat thing is a separate, Windows-only issue, to fix the fact that conda does not play well with the mess that Microsoft did with the new VS2017 directory tree. Instead, #53 is the issue that pkg_version.py is addressed to. Instead of using the obsolete __conda_version__, it uses load_setup_py_data() to extract the version info from CMakeLists.txt and RDKitUtils.cmake. I hope this makes things a bit clearer.

@greglandrum
Copy link
Member

is it ok if I let you guys figure this out and just wait for you to tell me what the answer is?
I wouldn't be sad to focus on some other stuff. :-)

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.

4 participants