Uploaded image for project: 'Apache Cordova'
  1. Apache Cordova
  2. CB-12163

plugin.xml 'resource-file' does not copy file to target for Windows

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: Master
    • Fix Version/s: None
    • Component/s: cordova-windows
    • Labels:
    • Environment:

      Windows 8/Windows10

      Description

      From plugin.xml:

      <resource-file src="windows/shared/myprops.properties" target="myprops.properties"/>

      <resource-file> does not copy the file from src to target anymore. It used to, but was changed. This creates a couple of issues since I expect it to be in the application, but it technically isn't. This means that I cannot update the file that doesn't exist and very much limits the functionality of <resource-file> (Also the documentation is wrong for Windows since it doesn't copy any files). Visual Studio also only shows the file from the src path. This doesn't make much sense since that file wouldn't be in the application.

      The expected behavior should be if the target is specified, <resource-file> should copy the file to the target location. In turn, Visual Studio should show the target file instead of the src file because the target is part of the application whereas the src file is not.

      This used to be expected behavior, but was changed as a result of CB-10326. The intention of that issue is limited to use cases of only having the src attribute, ignoring the possibility that we would want to update files in the target instead (which imo makes more sense). Resource files are not all .dll files, so it's not very user friendly to limit the <resource-file> tag to handling just those kinds of files.

      Proposed changes in JsprojManager.js and PluginHandler.js:
      If target attribute is specified, copy the file to the target and point the Include attribute to the path of the target.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/cordova-docs/pull/679

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/cordova-docs/pull/679
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit a2d5aa62d7215d01e886a85a568aef4f9fea808c in cordova-docs's branch refs/heads/master from Karen Tran
          [ https://git-wip-us.apache.org/repos/asf?p=cordova-docs.git;h=a2d5aa6 ]

          CB-12163 Add doc for reference attr for resource-file in Windows
          This closes #679

          Show
          jira-bot ASF subversion and git services added a comment - Commit a2d5aa62d7215d01e886a85a568aef4f9fea808c in cordova-docs's branch refs/heads/master from Karen Tran [ https://git-wip-us.apache.org/repos/asf?p=cordova-docs.git;h=a2d5aa6 ] CB-12163 Add doc for reference attr for resource-file in Windows This closes #679
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user ktop opened a pull request:

          https://github.com/apache/cordova-docs/pull/679

          CB-12163 Add doc for reference attr for resource-file in Windows

          <!--
          Please make sure the checklist boxes are all checked before submitting the PR. The checklist
          is intended as a quick reference, for complete details please see our Contributor Guidelines:

          http://cordova.apache.org/contribute/contribute_guidelines.html

          Thanks!
          -->

              1. Platforms affected
          • Windows
          • plugin.xml doc
              1. What does this PR do?
                Adds documentation for the new "reference" attribute for the resource-file tag in plugin.xml for Windows.
              1. What testing has been done on this change?
                N/A
              1. Checklist
          • [X ] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database
          • [X ] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
          • [X ] Added automated test coverage as appropriate for this change.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/ktop/cordova-docs cb-12163

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/cordova-docs/pull/679.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #679



          Show
          githubbot ASF GitHub Bot added a comment - GitHub user ktop opened a pull request: https://github.com/apache/cordova-docs/pull/679 CB-12163 Add doc for reference attr for resource-file in Windows <!-- Please make sure the checklist boxes are all checked before submitting the PR. The checklist is intended as a quick reference, for complete details please see our Contributor Guidelines: http://cordova.apache.org/contribute/contribute_guidelines.html Thanks! --> Platforms affected Windows plugin.xml doc What does this PR do? Adds documentation for the new "reference" attribute for the resource-file tag in plugin.xml for Windows. What testing has been done on this change? N/A Checklist [X ] [Reported an issue] ( http://cordova.apache.org/contribute/issues.html ) in the JIRA database [X ] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected. [X ] Added automated test coverage as appropriate for this change. You can merge this pull request into a Git repository by running: $ git pull https://github.com/ktop/cordova-docs cb-12163 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cordova-docs/pull/679.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #679
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vladimir-kotikov commented on the issue:

          https://github.com/apache/cordova-windows/pull/213

          @ktop, done!

          Show
          githubbot ASF GitHub Bot added a comment - Github user vladimir-kotikov commented on the issue: https://github.com/apache/cordova-windows/pull/213 @ktop, done!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/cordova-windows/pull/213

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/cordova-windows/pull/213
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 8f89a1f4d72d3422b8be7e29e714fb4fc5ae9e1c in cordova-windows's branch refs/heads/master from Karen Tran
          [ https://git-wip-us.apache.org/repos/asf?p=cordova-windows.git;h=8f89a1f ]

          CB-12163 Add resource-file reference functionality through a flag

          Show
          jira-bot ASF subversion and git services added a comment - Commit 8f89a1f4d72d3422b8be7e29e714fb4fc5ae9e1c in cordova-windows's branch refs/heads/master from Karen Tran [ https://git-wip-us.apache.org/repos/asf?p=cordova-windows.git;h=8f89a1f ] CB-12163 Add resource-file reference functionality through a flag
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit c71379a8e78542854597544fc97f756e2eae5b44 in cordova-windows's branch refs/heads/master from Karen Tran
          [ https://git-wip-us.apache.org/repos/asf?p=cordova-windows.git;h=c71379a ]

          CB-12163 Make resource-file copy files again

          Fix destPath check in copyFile

          Show
          jira-bot ASF subversion and git services added a comment - Commit c71379a8e78542854597544fc97f756e2eae5b44 in cordova-windows's branch refs/heads/master from Karen Tran [ https://git-wip-us.apache.org/repos/asf?p=cordova-windows.git;h=c71379a ] CB-12163 Make resource-file copy files again Fix destPath check in copyFile
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ktop commented on the issue:

          https://github.com/apache/cordova-windows/pull/213

          @daserge
          Now that there has been a cordova-common release, can we refresh cordova-common in cordova-windows and get this merged?

          Show
          githubbot ASF GitHub Bot added a comment - Github user ktop commented on the issue: https://github.com/apache/cordova-windows/pull/213 @daserge Now that there has been a cordova-common release, can we refresh cordova-common in cordova-windows and get this merged?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ktop commented on the issue:

          https://github.com/apache/cordova-windows/pull/213

          @daserge we need to wait for a cordova-common release since the commit I'm dependent on has been merged https://github.com/apache/cordova-lib/pull/509

          Show
          githubbot ASF GitHub Bot added a comment - Github user ktop commented on the issue: https://github.com/apache/cordova-windows/pull/213 @daserge we need to wait for a cordova-common release since the commit I'm dependent on has been merged https://github.com/apache/cordova-lib/pull/509
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user daserge commented on the issue:

          https://github.com/apache/cordova-windows/pull/213

          @ktop what is the status of this PR? Is it ready for merge?

          Show
          githubbot ASF GitHub Bot added a comment - Github user daserge commented on the issue: https://github.com/apache/cordova-windows/pull/213 @ktop what is the status of this PR? Is it ready for merge?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/cordova-lib/pull/509

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/cordova-lib/pull/509
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit b66ec165784fd9c196c9958c7e9baac7d25436f9 in cordova-lib's branch refs/heads/master from Karen Tran
          [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=b66ec16 ]

          CB-12163 Add reference attrib to resource-file for Windows

          This closes #509

          Show
          jira-bot ASF subversion and git services added a comment - Commit b66ec165784fd9c196c9958c7e9baac7d25436f9 in cordova-lib's branch refs/heads/master from Karen Tran [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=b66ec16 ] CB-12163 Add reference attrib to resource-file for Windows This closes #509
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vladimir-kotikov commented on the issue:

          https://github.com/apache/cordova-windows/pull/213

          Looks good 👍

          Show
          githubbot ASF GitHub Bot added a comment - Github user vladimir-kotikov commented on the issue: https://github.com/apache/cordova-windows/pull/213 Looks good 👍
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ktop commented on a diff in the pull request:

          https://github.com/apache/cordova-windows/pull/213#discussion_r92438038

          — Diff: template/cordova/lib/PluginHandler.js —
          @@ -53,18 +53,37 @@ var handlers = {
          },
          'resource-file':{
          install:function(obj, plugin, project, options) {

          • // do not copy, but reference the file in the plugin folder. This allows to
          • // have multiple source files map to the same target and select the appropriate
          • // one based on the current build settings, e.g. architecture.
          • // also, we don't check for existence. This allows to insert build variables
          • // into the source file name, e.g.
          • // <resource-file src="$(Platform)/My.dll" target="My.dll" />
          • var relativeSrcPath = getPluginFilePath(plugin, obj.src, project.projectFolder);
          • project.addResourceFileToProject(relativeSrcPath, obj.target, getTargetConditions(obj));
            + var targetConditions = getTargetConditions(obj);
            + if (targetConditions.reference) { + // do not copy, but reference the file in the plugin folder. This allows to + // have multiple source files map to the same target and select the appropriate + // one based on the current build settings, e.g. architecture. + // also, we don't check for existence. This allows to insert build variables + // into the source file name, e.g. + // <resource-file src="$(Platform)/My.dll" target="My.dll" /> + var relativeSrcPath = getPluginFilePath(plugin, obj.src, project.projectFolder); + project.addResourceFileToProject(relativeSrcPath, obj.target, targetConditions); + }

            else {
            + // if target already exists, emit warning to consider using a reference instead of copying
            + if (fs.existsSync(path.resolve(project.root, obj.target))) {
            + events.emit('warn', '<resource-file> with target ' + obj.target + ' already exists and will be overwritten ' +
            + 'by a <resource-file> with the same target. Consider using the attribute reference="true" in the ' +
            + '<resource-file> tag to avoid overwriting files with the same target. ');

              • End diff –

          I agree it would make it clearer for users.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ktop commented on a diff in the pull request: https://github.com/apache/cordova-windows/pull/213#discussion_r92438038 — Diff: template/cordova/lib/PluginHandler.js — @@ -53,18 +53,37 @@ var handlers = { }, 'resource-file':{ install:function(obj, plugin, project, options) { // do not copy, but reference the file in the plugin folder. This allows to // have multiple source files map to the same target and select the appropriate // one based on the current build settings, e.g. architecture. // also, we don't check for existence. This allows to insert build variables // into the source file name, e.g. // <resource-file src="$(Platform)/My.dll" target="My.dll" /> var relativeSrcPath = getPluginFilePath(plugin, obj.src, project.projectFolder); project.addResourceFileToProject(relativeSrcPath, obj.target, getTargetConditions(obj)); + var targetConditions = getTargetConditions(obj); + if (targetConditions.reference) { + // do not copy, but reference the file in the plugin folder. This allows to + // have multiple source files map to the same target and select the appropriate + // one based on the current build settings, e.g. architecture. + // also, we don't check for existence. This allows to insert build variables + // into the source file name, e.g. + // <resource-file src="$(Platform)/My.dll" target="My.dll" /> + var relativeSrcPath = getPluginFilePath(plugin, obj.src, project.projectFolder); + project.addResourceFileToProject(relativeSrcPath, obj.target, targetConditions); + } else { + // if target already exists, emit warning to consider using a reference instead of copying + if (fs.existsSync(path.resolve(project.root, obj.target))) { + events.emit('warn', '<resource-file> with target ' + obj.target + ' already exists and will be overwritten ' + + 'by a <resource-file> with the same target. Consider using the attribute reference="true" in the ' + + '<resource-file> tag to avoid overwriting files with the same target. '); End diff – I agree it would make it clearer for users.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vladimir-kotikov commented on a diff in the pull request:

          https://github.com/apache/cordova-windows/pull/213#discussion_r92344562

          — Diff: template/cordova/lib/JsprojManager.js —
          @@ -121,7 +121,12 @@ jsprojManager.prototype = {
          copyToOutputDirectory.text = 'Always';
          children.push(copyToOutputDirectory);

          • var item = createItemGroupElement('ItemGroup/Content', sourcePath, targetConditions, children);
            + var item;
              • End diff –

          IMO it'd be better to calculate path for `content` element outside this method (in `'resource-file'.install`) - this way you will avoid checking the same condition (`if (targetConditions.reference) ...`) twice and won't need to modify this method at all.

          Show
          githubbot ASF GitHub Bot added a comment - Github user vladimir-kotikov commented on a diff in the pull request: https://github.com/apache/cordova-windows/pull/213#discussion_r92344562 — Diff: template/cordova/lib/JsprojManager.js — @@ -121,7 +121,12 @@ jsprojManager.prototype = { copyToOutputDirectory.text = 'Always'; children.push(copyToOutputDirectory); var item = createItemGroupElement('ItemGroup/Content', sourcePath, targetConditions, children); + var item; End diff – IMO it'd be better to calculate path for `content` element outside this method (in `'resource-file'.install`) - this way you will avoid checking the same condition (`if (targetConditions.reference) ...`) twice and won't need to modify this method at all.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vladimir-kotikov commented on a diff in the pull request:

          https://github.com/apache/cordova-windows/pull/213#discussion_r92344997

          — Diff: template/cordova/lib/PluginHandler.js —
          @@ -53,18 +53,37 @@ var handlers = {
          },
          'resource-file':{
          install:function(obj, plugin, project, options) {

          • // do not copy, but reference the file in the plugin folder. This allows to
          • // have multiple source files map to the same target and select the appropriate
          • // one based on the current build settings, e.g. architecture.
          • // also, we don't check for existence. This allows to insert build variables
          • // into the source file name, e.g.
          • // <resource-file src="$(Platform)/My.dll" target="My.dll" />
          • var relativeSrcPath = getPluginFilePath(plugin, obj.src, project.projectFolder);
          • project.addResourceFileToProject(relativeSrcPath, obj.target, getTargetConditions(obj));
            + var targetConditions = getTargetConditions(obj);
            + if (targetConditions.reference) { + // do not copy, but reference the file in the plugin folder. This allows to + // have multiple source files map to the same target and select the appropriate + // one based on the current build settings, e.g. architecture. + // also, we don't check for existence. This allows to insert build variables + // into the source file name, e.g. + // <resource-file src="$(Platform)/My.dll" target="My.dll" /> + var relativeSrcPath = getPluginFilePath(plugin, obj.src, project.projectFolder); + project.addResourceFileToProject(relativeSrcPath, obj.target, targetConditions); + }

            else {
            + // if target already exists, emit warning to consider using a reference instead of copying
            + if (fs.existsSync(path.resolve(project.root, obj.target))) {
            + events.emit('warn', '<resource-file> with target ' + obj.target + ' already exists and will be overwritten ' +
            + 'by a <resource-file> with the same target. Consider using the attribute reference="true" in the ' +
            + '<resource-file> tag to avoid overwriting files with the same target. ');

              • End diff –

          Might be also useful to add that adding `reference` will not copy user files to destination directory. WDYT?

          Show
          githubbot ASF GitHub Bot added a comment - Github user vladimir-kotikov commented on a diff in the pull request: https://github.com/apache/cordova-windows/pull/213#discussion_r92344997 — Diff: template/cordova/lib/PluginHandler.js — @@ -53,18 +53,37 @@ var handlers = { }, 'resource-file':{ install:function(obj, plugin, project, options) { // do not copy, but reference the file in the plugin folder. This allows to // have multiple source files map to the same target and select the appropriate // one based on the current build settings, e.g. architecture. // also, we don't check for existence. This allows to insert build variables // into the source file name, e.g. // <resource-file src="$(Platform)/My.dll" target="My.dll" /> var relativeSrcPath = getPluginFilePath(plugin, obj.src, project.projectFolder); project.addResourceFileToProject(relativeSrcPath, obj.target, getTargetConditions(obj)); + var targetConditions = getTargetConditions(obj); + if (targetConditions.reference) { + // do not copy, but reference the file in the plugin folder. This allows to + // have multiple source files map to the same target and select the appropriate + // one based on the current build settings, e.g. architecture. + // also, we don't check for existence. This allows to insert build variables + // into the source file name, e.g. + // <resource-file src="$(Platform)/My.dll" target="My.dll" /> + var relativeSrcPath = getPluginFilePath(plugin, obj.src, project.projectFolder); + project.addResourceFileToProject(relativeSrcPath, obj.target, targetConditions); + } else { + // if target already exists, emit warning to consider using a reference instead of copying + if (fs.existsSync(path.resolve(project.root, obj.target))) { + events.emit('warn', '<resource-file> with target ' + obj.target + ' already exists and will be overwritten ' + + 'by a <resource-file> with the same target. Consider using the attribute reference="true" in the ' + + '<resource-file> tag to avoid overwriting files with the same target. '); End diff – Might be also useful to add that adding `reference` will not copy user files to destination directory. WDYT?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vladimir-kotikov commented on a diff in the pull request:

          https://github.com/apache/cordova-windows/pull/213#discussion_r92345318

          — Diff: template/cordova/lib/PluginHandler.js —
          @@ -188,7 +207,7 @@ module.exports.getUninstaller = function(type) {
          };

          function getTargetConditions(obj) {

          • return { versions: obj.versions, deviceTarget: obj.deviceTarget, arch: obj.arch }

            ;
            + return

            { versions: obj.versions, deviceTarget: obj.deviceTarget, arch: obj.arch, reference: obj.reference }

            ;

              • End diff –

          As per comments above `reference` field is not really required - i's easier to use `obj.reference` directly

          Show
          githubbot ASF GitHub Bot added a comment - Github user vladimir-kotikov commented on a diff in the pull request: https://github.com/apache/cordova-windows/pull/213#discussion_r92345318 — Diff: template/cordova/lib/PluginHandler.js — @@ -188,7 +207,7 @@ module.exports.getUninstaller = function(type) { }; function getTargetConditions(obj) { return { versions: obj.versions, deviceTarget: obj.deviceTarget, arch: obj.arch } ; + return { versions: obj.versions, deviceTarget: obj.deviceTarget, arch: obj.arch, reference: obj.reference } ; End diff – As per comments above `reference` field is not really required - i's easier to use `obj.reference` directly
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vladimir-kotikov commented on a diff in the pull request:

          https://github.com/apache/cordova-windows/pull/213#discussion_r92344993

          — Diff: template/cordova/lib/PluginHandler.js —
          @@ -53,18 +53,37 @@ var handlers = {
          },
          'resource-file':{
          install:function(obj, plugin, project, options) {

          • // do not copy, but reference the file in the plugin folder. This allows to
          • // have multiple source files map to the same target and select the appropriate
          • // one based on the current build settings, e.g. architecture.
          • // also, we don't check for existence. This allows to insert build variables
          • // into the source file name, e.g.
          • // <resource-file src="$(Platform)/My.dll" target="My.dll" />
          • var relativeSrcPath = getPluginFilePath(plugin, obj.src, project.projectFolder);
          • project.addResourceFileToProject(relativeSrcPath, obj.target, getTargetConditions(obj));
            + var targetConditions = getTargetConditions(obj);
            + if (targetConditions.reference) {
              • End diff –

          I think that the object that `getTargetConditions` returns was originally meant to hold details about target platform (Win8.1, WP8.1, Win10) and architecture to create a `condition` attribute, so `reference` field doesn't really fit here - you might just check `if (obj.reference) ...`

          Show
          githubbot ASF GitHub Bot added a comment - Github user vladimir-kotikov commented on a diff in the pull request: https://github.com/apache/cordova-windows/pull/213#discussion_r92344993 — Diff: template/cordova/lib/PluginHandler.js — @@ -53,18 +53,37 @@ var handlers = { }, 'resource-file':{ install:function(obj, plugin, project, options) { // do not copy, but reference the file in the plugin folder. This allows to // have multiple source files map to the same target and select the appropriate // one based on the current build settings, e.g. architecture. // also, we don't check for existence. This allows to insert build variables // into the source file name, e.g. // <resource-file src="$(Platform)/My.dll" target="My.dll" /> var relativeSrcPath = getPluginFilePath(plugin, obj.src, project.projectFolder); project.addResourceFileToProject(relativeSrcPath, obj.target, getTargetConditions(obj)); + var targetConditions = getTargetConditions(obj); + if (targetConditions.reference) { End diff – I think that the object that `getTargetConditions` returns was originally meant to hold details about target platform (Win8.1, WP8.1, Win10) and architecture to create a `condition` attribute, so `reference` field doesn't really fit here - you might just check `if (obj.reference) ...`
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user ktop opened a pull request:

          https://github.com/apache/cordova-lib/pull/509

          CB-12163 Add reference attrib to resource-file for Windows

          <!--
          Please make sure the checklist boxes are all checked before submitting the PR. The checklist
          is intended as a quick reference, for complete details please see our Contributor Guidelines:

          http://cordova.apache.org/contribute/contribute_guidelines.html

          Thanks!
          -->

              1. Platforms affected
                cordova-common for Windows
              1. What does this PR do?
                Adds a new attribute to the resource-file tag for Windows. The attribute is for moving the current functionality of "referencing files" to be switched on by this flag in order to bring back the copy functionality to be default for resource-file.

          ex.
          ```
          <resource-file src="x86/foo.dll" target="foo.dll" arch="x86" reference="true" />
          <resource-file src="x64/foo.dll" target="foo.dll" arch="x64" reference="true" />
          ```

              1. What testing has been done on this change?
                Manual testing and unit testing. Need to pull in changes from the cordova-windows pull request to get it to work properly.
              1. Checklist
          • [X] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database
          • [X] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
          • [X] Added automated test coverage as appropriate for this change.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/ktop/cordova-lib cb12163

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/cordova-lib/pull/509.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #509


          commit 9d69d3df1d765ad9c05ece48864fc47c6c3e7061
          Author: ktop <ktop500@gmail.com>
          Date: 2016-12-12T19:35:04Z

          CB-12163 Add reference attrib to resource-file for Windows


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user ktop opened a pull request: https://github.com/apache/cordova-lib/pull/509 CB-12163 Add reference attrib to resource-file for Windows <!-- Please make sure the checklist boxes are all checked before submitting the PR. The checklist is intended as a quick reference, for complete details please see our Contributor Guidelines: http://cordova.apache.org/contribute/contribute_guidelines.html Thanks! --> Platforms affected cordova-common for Windows What does this PR do? Adds a new attribute to the resource-file tag for Windows. The attribute is for moving the current functionality of "referencing files" to be switched on by this flag in order to bring back the copy functionality to be default for resource-file. ex. ``` <resource-file src="x86/foo.dll" target="foo.dll" arch="x86" reference="true" /> <resource-file src="x64/foo.dll" target="foo.dll" arch="x64" reference="true" /> ``` What testing has been done on this change? Manual testing and unit testing. Need to pull in changes from the cordova-windows pull request to get it to work properly. Checklist [X] [Reported an issue] ( http://cordova.apache.org/contribute/issues.html ) in the JIRA database [X] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected. [X] Added automated test coverage as appropriate for this change. You can merge this pull request into a Git repository by running: $ git pull https://github.com/ktop/cordova-lib cb12163 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cordova-lib/pull/509.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #509 commit 9d69d3df1d765ad9c05ece48864fc47c6c3e7061 Author: ktop <ktop500@gmail.com> Date: 2016-12-12T19:35:04Z CB-12163 Add reference attrib to resource-file for Windows
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ktop commented on the issue:

          https://github.com/apache/cordova-windows/pull/213

          @daserge Thanks for the bump, Tim's email was in my spam. Yes, this would be a breaking change as it is now. We should wait until we come to a final decision before proceeding with this pr.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ktop commented on the issue: https://github.com/apache/cordova-windows/pull/213 @daserge Thanks for the bump, Tim's email was in my spam. Yes, this would be a breaking change as it is now. We should wait until we come to a final decision before proceeding with this pr.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user daserge commented on the issue:

          https://github.com/apache/cordova-windows/pull/213

          @ktop, could you please comment on @timbarham message?
          http://markmail.org/message/2fproqgmbrc2gxpz
          Is this a breaking change?

          Show
          githubbot ASF GitHub Bot added a comment - Github user daserge commented on the issue: https://github.com/apache/cordova-windows/pull/213 @ktop, could you please comment on @timbarham message? http://markmail.org/message/2fproqgmbrc2gxpz Is this a breaking change?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ktop commented on the issue:

          https://github.com/apache/cordova-windows/pull/213

          @vladimir-kotikov Can you review? This pr reverts resource-file to what it used to do so it will copy files.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ktop commented on the issue: https://github.com/apache/cordova-windows/pull/213 @vladimir-kotikov Can you review? This pr reverts resource-file to what it used to do so it will copy files.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user codecov-io commented on the issue:

          https://github.com/apache/cordova-windows/pull/213

            1. [Current coverage](https://codecov.io/gh/apache/cordova-windows/pull/213?src=pr) is 76.10% (diff: 100%)
              > Merging 213(https://codecov.io/gh/apache/cordova-windows/pull/213?src=pr) into [master](https://codecov.io/gh/apache/cordova-windows/branch/master?src=pr) will decrease coverage by *0.03%*

          ```diff
          @@ master #213 diff @@
          ==========================================
          Files 16 16
          Lines 2204 2201 -3
          Methods 413 412 -1
          Messages 0 0
          Branches 430 430
          ==========================================

          • Hits 1678 1675 -3
            Misses 526 526
            Partials 0 0
            ```

          > Powered by [Codecov](https://codecov.io?src=pr). Last update [623ff18...d00e0c3](https://codecov.io/gh/apache/cordova-windows/compare/623ff181dbb780537c9a9b646db6f72bd59722e9...d00e0c33d388f410d6444920d72b7ac1e86df3ef?src=pr)

          Show
          githubbot ASF GitHub Bot added a comment - Github user codecov-io commented on the issue: https://github.com/apache/cordova-windows/pull/213 [Current coverage] ( https://codecov.io/gh/apache/cordova-windows/pull/213?src=pr ) is 76.10% (diff: 100%) > Merging 213 ( https://codecov.io/gh/apache/cordova-windows/pull/213?src=pr ) into [master] ( https://codecov.io/gh/apache/cordova-windows/branch/master?src=pr ) will decrease coverage by * 0.03% * ```diff @@ master #213 diff @@ ========================================== Files 16 16 Lines 2204 2201 -3 Methods 413 412 -1 Messages 0 0 Branches 430 430 ========================================== Hits 1678 1675 -3 Misses 526 526 Partials 0 0 ``` > Powered by [Codecov] ( https://codecov.io?src=pr ). Last update [623ff18...d00e0c3] ( https://codecov.io/gh/apache/cordova-windows/compare/623ff181dbb780537c9a9b646db6f72bd59722e9...d00e0c33d388f410d6444920d72b7ac1e86df3ef?src=pr )
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user ktop opened a pull request:

          https://github.com/apache/cordova-windows/pull/213

          CB-12163 Make resource-file copy files again

          <!--
          Please make sure the checklist boxes are all checked before submitting the PR. The checklist
          is intended as a quick reference, for complete details please see our Contributor Guidelines:

          http://cordova.apache.org/contribute/contribute_guidelines.html

          Thanks!
          -->

              1. Platforms affected
          • Windows
              1. What does this PR do?
          • Revert the functionality of resource-file in plugin.xml. It will now copy files again instead of using the references.
              1. What testing has been done on this change?
          • Updated the spec tests and ran them
          • Created a new app with the updated resource-file and check to see if it copies files.
              1. Checklist
          • [X] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database
          • [X] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
          • [X] Added automated test coverage as appropriate for this change.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/ktop/cordova-windows cb12163

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/cordova-windows/pull/213.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #213


          commit d00e0c33d388f410d6444920d72b7ac1e86df3ef
          Author: ktop <ktop500@gmail.com>
          Date: 2016-12-06T20:58:42Z

          CB-12163 Make resource-file copy files again


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user ktop opened a pull request: https://github.com/apache/cordova-windows/pull/213 CB-12163 Make resource-file copy files again <!-- Please make sure the checklist boxes are all checked before submitting the PR. The checklist is intended as a quick reference, for complete details please see our Contributor Guidelines: http://cordova.apache.org/contribute/contribute_guidelines.html Thanks! --> Platforms affected Windows What does this PR do? Revert the functionality of resource-file in plugin.xml. It will now copy files again instead of using the references. What testing has been done on this change? Updated the spec tests and ran them Created a new app with the updated resource-file and check to see if it copies files. Checklist [X] [Reported an issue] ( http://cordova.apache.org/contribute/issues.html ) in the JIRA database [X] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected. [X] Added automated test coverage as appropriate for this change. You can merge this pull request into a Git repository by running: $ git pull https://github.com/ktop/cordova-windows cb12163 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cordova-windows/pull/213.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #213 commit d00e0c33d388f410d6444920d72b7ac1e86df3ef Author: ktop <ktop500@gmail.com> Date: 2016-12-06T20:58:42Z CB-12163 Make resource-file copy files again
          Hide
          ktop500 Karen Tran added a comment -

          I think there will need to be some sort of compromise to this issue. This little snippet was found in PluginHandler.js and it contradicts with what I am proposing. I wouldn't be able to simply check that a target path exists.

          // do not copy, but reference the file in the plugin folder. This allows to
          // have multiple source files map to the same target and select the appropriate
          // one based on the current build settings, e.g. architecture.
          // also, we don't check for existence. This allows to insert build variables
          // into the source file name, e.g.
          // <resource-file src="$(Platform)/My.dll" target="My.dll" />
          

          One way or another, the solution would end up having a Windows quirk and should be properly documented, which it isn't now.

          A couple things we need to decide on:
          1. What should be the default behavior of <resource-file> tag? Should it simply be copy resources as it was originally intended to, or should it be doing what it is now, which is making a reference to the resource files.
          2. Should <resource-file> tag handle both functionalities, or should one be separated out into another tag?

          Show
          ktop500 Karen Tran added a comment - I think there will need to be some sort of compromise to this issue. This little snippet was found in PluginHandler.js and it contradicts with what I am proposing. I wouldn't be able to simply check that a target path exists. // do not copy, but reference the file in the plugin folder. This allows to // have multiple source files map to the same target and select the appropriate // one based on the current build settings, e.g. architecture. // also, we don't check for existence. This allows to insert build variables // into the source file name, e.g. // <resource-file src= "$(Platform)/My.dll" target= "My.dll" /> One way or another, the solution would end up having a Windows quirk and should be properly documented, which it isn't now. A couple things we need to decide on: 1. What should be the default behavior of <resource-file> tag? Should it simply be copy resources as it was originally intended to, or should it be doing what it is now, which is making a reference to the resource files. 2. Should <resource-file> tag handle both functionalities, or should one be separated out into another tag?

            People

            • Assignee:
              ktop500 Karen Tran
              Reporter:
              ktop500 Karen Tran
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development