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

upload based on manifest diff, not snapshot #3480

Merged
merged 9 commits into from Sep 13, 2018

Conversation

@memsharded
Copy link
Member

@memsharded memsharded commented Sep 4, 2018

This could be a fix for:

This is the implementation for API V1.

Changelog: Feature: The upload of files now uses the conanmanifest.txt file to know if a file has to be uploaded or not. It avoid associated issues with the metainformation of the files permissions contained in the tgz files.

@ghost ghost assigned memsharded Sep 4, 2018
@ghost ghost added the stage: review label Sep 4, 2018
@ghost ghost assigned memsharded Sep 6, 2018
@memsharded memsharded requested a review from jgsogo Sep 6, 2018
@@ -84,7 +83,7 @@ def _upload(self, conan_file, conan_ref, force, packages_ids, retry, retry_wait,
else: # Or use the default otherwise
upload_remote = self._registry.default_remote

if not force:
if no_overwrite != "force":
Copy link
Member

@danimtb danimtb Sep 6, 2018

This comparison is a bit confusing. I know doing it this way there is one parameter less but I think the old code reads better IMO

Copy link
Member Author

@memsharded memsharded Sep 6, 2018

Yeah, I know, but the alternative is passing around 2 mutually exclusive arguments, which is bad interface. I thought changing both for a policy, but wanted to keep changes to a minimum.

Copy link
Contributor

@lasote lasote Sep 6, 2018

introduce the policy please, I really hate that kind of arguments

Copy link
Member

@jgsogo jgsogo left a comment

Also, I will rename no_overwrite variable, I find it hard to follow its semantics... it a string that can hold values in all, recipe, force?

for filename in new + modified}
for filename in local_snapshot}

deleted = set(remote_snapshot).difference(local_snapshot)
Copy link
Member

@jgsogo jgsogo Sep 6, 2018

remote_snapshot keys have backward slashes?

Copy link
Member Author

@memsharded memsharded Sep 6, 2018

If they are there, better leave them, there might be some reason.

if no_overwrite in ("all", "recipe"):
raise ConanException("Local recipe is different from the remote recipe. "
"Forbidden overwrite")

local_snapshot = {filename: None for filename, abs_path in the_files.items()}
Copy link
Member

@jgsogo jgsogo Sep 6, 2018

local_snapshot = set(the_files.keys()) (or leave it as an iterable)

if no_overwrite == "all":
raise ConanException("Local package is different from the remote package. "
"Forbidden overwrite")

files_to_upload = {filename: the_files[filename] for filename in new + modified}
local_snapshot = {filename: None for filename, abs_path in the_files.items()}
Copy link
Member

@jgsogo jgsogo Sep 6, 2018

local_snapshot = set(the_files.keys()) (or leave it as an iterable, same as above)

if no_overwrite == "all":
raise ConanException("Local package is different from the remote package. "
"Forbidden overwrite")

files_to_upload = {filename: the_files[filename] for filename in new + modified}
local_snapshot = {filename: None for filename, abs_path in the_files.items()}
files_to_upload = {filename: the_files[filename] for filename in local_snapshot}
Copy link
Member

@jgsogo jgsogo Sep 6, 2018

files_to_upload is not just the same dictionary as the_files?

files_to_upload = {filename.replace("\\", "/"): the_files[filename]
for filename in new + modified}
for filename in local_snapshot}
Copy link
Member

@jgsogo jgsogo Sep 6, 2018

files_to_upload = {filename.replace("\\", "/"): abs_path for filename, abs_path in the_files.items()}

And I'm also wondering if this replacement is needed at all

Copy link
Member Author

@memsharded memsharded Sep 6, 2018

Just, it is a bit ugly, but I prefer to leave it by now, too many changes.

lasote
lasote approved these changes Sep 7, 2018
Copy link
Contributor

@lasote lasote left a comment

It changes again the conan_api.upload signature and the cpt could became very messy if we don't pin the "conan" dependency, let's talk about it.

danimtb
danimtb approved these changes Sep 7, 2018
@@ -1007,14 +1009,30 @@ def upload(self, *args):
cwd = os.getcwd()
info = None

if args.force and args.no_overwrite:
raise ConanException("'no_overwrite' argument cannot be used together with 'force'")
Copy link
Member

@danimtb danimtb Sep 7, 2018

no_overwrite was the notation used to raise from conan API. I would indicate here --no-overwrite to have a more clear message. Same for skip_upload and force -- > --skip-upload & --force

Copy link
Member Author

@memsharded memsharded Sep 12, 2018

Done

@memsharded memsharded added this to the 1.8 milestone Sep 12, 2018
@lasote lasote merged commit 9a9a0c4 into conan-io:develop Sep 13, 2018
2 checks passed
@ghost ghost removed the stage: review label Sep 13, 2018
@memsharded memsharded deleted the feature/upload_manifest_diff branch Sep 17, 2018
grisumbras pushed a commit to grisumbras/conan that referenced this issue Dec 27, 2018
* upload based on manifest diff, not snapshot

* same for packages

* cleaning unused code

* fix rest api test

* upload --force forces actual upload

* using upload policies

* simplified upload

* fixed error

* arguments incompat messages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants