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

finalizing paths.json spec #1535

Merged
merged 30 commits into from
Dec 16, 2016
Merged

finalizing paths.json spec #1535

merged 30 commits into from
Dec 16, 2016

Conversation

soapy1
Copy link
Contributor

@soapy1 soapy1 commented Nov 15, 2016

Creating a new pr to address the code review from #1501 (review)

This is dependent on conda/conda#3887 being merged

prefix_placeholder, file_mode = has_prefix(fi, files_with_prefix)
path = os.path.join(prefix, fi)
file_info = {
"short_path": get_short_path(m, fi),
"sha256": sha256_checksum(path),
"size_in_bytes": os.path.getsize(path),
"file_type": getattr(file_type(path), "name"),
"node_type": node_type(path).name,
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Remove .name from the line above.
  2. The class below is from conda._vendor.auxlib.entity. Let's put it in conda_interface.py.
class EntityEncoder(JSONEncoder):
    # json.dumps(obj, cls=SetEncoder)
    def default(self, obj):
       if hasattr(obj, 'dump'):
          return obj.dump()
       elif hasattr(obj, '__json__'):
          return obj.__json__()
       elif hasattr(obj, 'to_json'):
          return obj.to_json()
       elif hasattr(obj, 'as_json'):
          return obj.as_json()
       return JSONEncoder.default(self, obj)
  1. Add a __json__ method to NodeType that returns self.name. See other enums in conda/base/constants.py for examples.

@@ -123,3 +125,40 @@ def which_prefix(path):
# we cannot chop off any more directories, so we didn't find it
return None
prefix = dirname(prefix)


class NodeType(Enum):
Copy link
Contributor

Choose a reason for hiding this comment

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

Above there's a line

if parse_version(conda.__version__) >= parse_version("4.2"):

We should also have a section for 4.3 where we actually do the real imports from conda/exports.py. Then add this new stuff under the else after 4.3. That way it's easy to rip out in the future.

Copy link
Contributor

@kalefranz kalefranz left a comment

Choose a reason for hiding this comment

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

LGTM

@soapy1
Copy link
Contributor Author

soapy1 commented Nov 16, 2016

I think these errors are the same kind that we were seeing in conda and has to do with updates to a new version of requests

@soapy1 soapy1 added the 4_Needs_Review [deprecated] use milestones/project boards label Nov 18, 2016
@msarahan
Copy link
Contributor

Please merge master to resolve the conflict. Let me know if you have any questions.

@kalefranz
Copy link
Contributor

@soapy1 let's get master merged in, see if we need fixes, and then get it merged

@kalefranz
Copy link
Contributor

Wait, how did soapy1#14 get merged, but I don't see the commit?

@soapy1
Copy link
Contributor Author

soapy1 commented Nov 28, 2016

Oh, it looks like the changes aren't in here either. I had to rebase on master to resolve some merge conflicts, could that have caused it?
I'm going to merge it in again.

@soapy1 soapy1 force-pushed the files-json-review branch 2 times, most recently from 97978fc to e6718bb Compare November 28, 2016 21:23
@soapy1 soapy1 force-pushed the files-json-review branch 4 times, most recently from 19154ef to dd9a94a Compare December 15, 2016 19:56
@msarahan
Copy link
Contributor

Appveyor failure: https://ci.appveyor.com/project/ContinuumAnalyticsFOSS/conda-build/build/1.0.1069/job/0v2qc287b1gnl2d5#L1063

Looks like python 2.7 does not define os.link in windows.

@soapy1
Copy link
Contributor Author

soapy1 commented Dec 15, 2016

conda canary test is failing because conda 4.3 isn't actually exporting FileMode this must have slipped through the cracks.
Submitted a pr to fix this: conda/conda#4080

@kalefranz
Copy link
Contributor

Looks like python 2.7 does not define os.link in windows.

I guess it's not yet so, but here it seems appropriate to export create_link() from conda/gateways/disk/create.py.

@soapy1
Copy link
Contributor Author

soapy1 commented Dec 16, 2016

@kalefranz I tried to get around that problem by having another test on windows that doesn't need link: https://github.com/conda/conda-build/pull/1535/files#diff-ba96ca525f7bacd1a0ee8b10d71d6b2bR63
Do you think this is a sufficient test, or should I import create_link from conda?

assert 2 == CrossPlatformStLink.st_nlink(test_file_linked)


def test_crossplatform_st_link_on_win(tmpdir):
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want a platform skip on things other than windows. Just invert the logic from your test one up.

@msarahan
Copy link
Contributor

Current windows test failures are other things that I'm looking into. This seems fine and ready to go to me.

@msarahan
Copy link
Contributor

Thanks @soapy1 - merging.

@github-actions
Copy link

github-actions bot commented May 9, 2022

Hi there, thank you for your contribution!

This pull request has been automatically locked because it has not had recent activity after being closed.

Please open a new issue or pull request if needed.

Thanks!

@github-actions github-actions bot added the locked [bot] locked due to inactivity label May 9, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4_Needs_Review [deprecated] use milestones/project boards locked [bot] locked due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants