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

remove info/recipe.json from conda package #781

Merged
merged 4 commits into from
Jun 12, 2016
Merged

Conversation

ilanschnell
Copy link
Contributor

This PR removes info/recipe.json from the conda package which conda-build creates. I have never understood the purpose of this file, because info/recipe/ contains the entire recipe anyway. Given that the inclusion of the original recipe into a conda package is optional anyway, I don't see any risk of this change breaking anything.

CC @kalefranz

@ilanschnell
Copy link
Contributor Author

@kalefranz can this be merged?

@kalefranz
Copy link
Contributor

Tests pass. For the most part anyway (need to fixup this repo like I did conda/conda). Anyway, not this PR's fault. But removing this file, I'm only afraid of what we might break. Both for us and for people who might be extending conda somehow--even in undocumented ways.

For this PR in particular, I would propose we figure out things we might want in a conda-build 2.0 and then wait to merge until that time. Thoughts?

@ilanschnell
Copy link
Contributor Author

Ok, it can wait. I just hate the recipe.json so much that I would rather see it gone sooner than later :-)

@kalefranz
Copy link
Contributor

I know you hate it. :)

But it's also not technically causing any harm right now, so we should patiently deprecate :)

On Feb 22, 2016, at 11:54 PM, Ilan Schnell notifications@github.com wrote:

Ok, it can wait. I just hate the recipe.json so much that I would rather see it gone sooner than later :-)


Reply to this email directly or view it on GitHub.

@pelson
Copy link
Contributor

pelson commented Feb 23, 2016

For this PR in particular, I would propose we figure out things we might want in a conda-build 2.0 and then wait to merge until that time. Thoughts?

Sounds like you might be on the verge of adopting semver? My advice, FWIW: release a 2.0 soon, then release a 3.0 a month after to get you over the hurdle of major releases being a big deal (of the scipy stack only Jupyter seems to have transcended beyond the v3's).

No matter which version you do it in, a major change is unpleasant if it is undocumented and breaks things. It often requires more effort to debug than it takes to implement the thing in the first place and for that reason, no matter what you choose, I'd encourage you to add this to the changelog (not sure how that is normally managed, is that curated at release time?).

Anyway, 👍 on the change.

@ilanschnell
Copy link
Contributor Author

Yeah, thanks for your support @pelson! Having the recipe in the conda package is optional anyway, because of the --no-include-recipe command line option, so nothing will break.

@groutr
Copy link
Contributor

groutr commented Mar 9, 2016

I agree with deprecation of it.

@stuarteberg
Copy link
Contributor

I have never understood the purpose of this file, because info/recipe/ contains the entire recipe anyway.

There is one difference between info/recipe/meta.yaml and info/recipe.json, which is that recipe.json reflects the final value of all recipe fields, after selector processing and jinja template evaluation.

For example, in meta.yaml

build:
  number: 23
  string: np{{np}}py{{py}}_{{PKG_BUILDNUM}}_g{{GIT_FULL_HASH[:7]}}
...

In recipe.json:

{
  "build": {
    "number": "23",
    "string": "np19py27_23_g25e868b",
    ...
  }
  ...
}

OK, it's true that a lot of the important info is also available in info/index.json, too:

{
  "build": "np19py27_23_g25e868b",
  "build_number": 23,
  ...
}

... but recipe.json might still be useful if it contains an evaluated jinja template (or selector) in a field that doesn't get copied into index.json.

One likely place to find such cases is the extra section of meta.yaml:

source:
  git_url: ../
  git_rev: HEAD

build:
  number: 23
  string: np{{np}}py{{py}}_{{PKG_BUILDNUM}}

...

extra:
  built_by: {{USER}}
  from_commit: {{GIT_FULL_HASH}}

...which results in the following recipe.json:

{
  "source": {
    "git_rev": "HEAD",
    "git_url": "../",
    "md5": "",
    "patches": [],
    "path": "",
    "svn_rev": ""
  },
  "build": {
    "number": "23",
    "string": "np19py27_23",
    ...
  },
  "extra": {
    "built_by": "bergs",
    "from_commit": "88d0454b44f006e9f8963ec3188d42e8b9243ecd"
  },
...
}

@groutr groutr added the pending::discussion contains some ongoing discussion that needs to be resolved prior to proceeding label Mar 10, 2016
@groutr
Copy link
Contributor

groutr commented Mar 10, 2016

@stuarteberg That is a really interesting point.

@msarahan
Copy link
Contributor

msarahan commented May 5, 2016

@stuarteberg - should we replace recipe.json with a static, rendered meta.yaml in the embedded recipe? Is there value in embedding a raw template meta.yaml?

@stuarteberg
Copy link
Contributor

should we replace recipe.json with a static, rendered meta.yaml in the embedded recipe? Is there value in embedding a raw template meta.yaml?

I don't have a particularly strong feeling about this. I guess if size were no concern, I'd put the whole enchilada into the package -- meta.yaml, build.sh, test scripts, patches, etc. It won't be more than a couple KB, and it makes debugging clearer.

But if that's not an option, I guess I'd prefer to have the fully-rendered version, as we have now. It's a little weird that it's provided as json instead of yaml, but maybe that makes sense since json has better support in most languages. The only thing I don't like is that it's called recipe.json instead of meta.json.

@msarahan
Copy link
Contributor

msarahan commented May 5, 2016

What I meant was with #908, we can now output a rendered yaml file. I would save it as meta.yaml, and not include the raw meta.yaml at all. It makes some sense to me - the meta.yaml template does not say what values the recipe actually used.

@stuarteberg
Copy link
Contributor

Sounds good to me.

@msarahan msarahan added this to the 2.0 milestone May 6, 2016
@msarahan
Copy link
Contributor

code for this proposed static meta.yaml file is at https://github.com/conda/conda-build/pull/950/files#diff-ac55d744935b1f37a3369258b88e8f1fR189 - please let me know what you think. I tried to add a bit to help keep track of where meta.yaml came from.

Will it be an issue that we always include this, even when the user specifies to not include the recipe? We don't include build.sh or bld.bat, but might meta.yaml contain sensitive or proprietary information?

@msarahan
Copy link
Contributor

@ilanschnell - if you want to still get this PR in, would you mind fixing the merge conflicts?

I can also just remove this file myself. We now have the original recipe in meta.yaml, and the rendered recipe in meta.yaml.rendered, both in the info/recipe folder. See #1004

@ilanschnell
Copy link
Contributor Author

@msarahan conflicts resolved

@msarahan msarahan merged commit 758f760 into master Jun 12, 2016
@msarahan
Copy link
Contributor

Thanks! I have merged this into the 1.21.x branch as well.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked [bot] locked due to inactivity pending::discussion contains some ongoing discussion that needs to be resolved prior to proceeding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants