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 output warning in MSBuild #4518

Merged

Conversation

Projects
None yet
4 participants
@memsharded
Copy link
Contributor

commented Feb 13, 2019

Changelog: Fix: Remove output warnings in MSBuild helper.
Docs: Omit

@ghost ghost assigned memsharded Feb 13, 2019

@ghost ghost added the stage: review label Feb 13, 2019

@memsharded memsharded added this to the 1.13 milestone Feb 13, 2019

@memsharded memsharded assigned jgsogo and unassigned memsharded Feb 13, 2019

@jgsogo

jgsogo approved these changes Feb 15, 2019

Copy link
Member

left a comment

👍

@jgsogo

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

Ready to merge, just wondering if we should merge it to develop or to release/1.12.3 if we are going to release that minor one.

@ghost ghost assigned danimtb Feb 15, 2019

@danimtb danimtb modified the milestones: 1.13, 1.12.3 Feb 15, 2019

@danimtb danimtb changed the base branch from develop to release/1.12.3 Feb 15, 2019

@danimtb danimtb changed the base branch from release/1.12.3 to develop Feb 15, 2019

@danimtb danimtb force-pushed the memsharded:feature/remove_output_warning branch from c520936 to d5fffc8 Feb 15, 2019

@danimtb danimtb changed the base branch from develop to release/1.12.3 Feb 15, 2019

@@ -11,6 +11,7 @@
from conans.errors import ConanException
from conans.model.conan_file import ConanFile
from conans.model.version import Version
from conans.tools import vcvars_command as tools_vcvars_command

This comment has been minimized.

Copy link
@memsharded

memsharded Feb 16, 2019

Author Contributor

Didn't we say that conan does NOT import from conans.tools and that is exclusively for users/recipes? Besides 2 remaining things, that should be removed, that is the way it is now.

This comment has been minimized.

Copy link
@jgsogo

jgsogo Feb 18, 2019

Member

@danimtb pointed out that it is much more important not to expose an output argument in the public API/tool. I agree with him, the signature for MSBuild::get_version() cannot require the user to provide an output.

This comment has been minimized.

Copy link
@memsharded

memsharded Feb 18, 2019

Author Contributor

The problem is that MSBuild.get_version() prints that ugly warning, that users cannot fix.
Also the MSBuild binary_log feature, uses internally this functionality, printing the ugly warning message, and that cannot be fixed either by the user.

This has to be fixed, outputting warnings that the user cannot fix doesn't make sense, no matter how "clean" our api is.

This comment has been minimized.

Copy link
@jgsogo

jgsogo Feb 18, 2019

Member

The patch by @danimtb avoids that warning (secondary goal) and do not expose the output argument (main goal).

@lasote lasote merged commit 61fcee6 into conan-io:release/1.12.3 Feb 18, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Feb 18, 2019

@memsharded memsharded deleted the memsharded:feature/remove_output_warning branch Feb 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.