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

CB-8123 Plugin references can target specific windows platforms. #155

Closed
wants to merge 6 commits into from
Closed

CB-8123 Plugin references can target specific windows platforms. #155

wants to merge 6 commits into from

Conversation

TimBarham
Copy link
Contributor

Adds support for target, versions and arch attributes on <lib-file> and <framework> elements in the windows platform of plugin.xml. This allows plugin authors to target different references to different target platforms.

Also adds support for src attribute as an alias for the Include attribute on the <lib-file> element (since src is documented, but Include is used by existing plugins).

Adds some tests to cover the new attributes. Updates existing plugin tests for windows8 platform to also test windows platform (left in windows8 tests to help verify backward compatibility with old windows8 platform).

As part of this change, refactored jsproj to jsprojManager to reflect the fact that, with the windows platform, this class now manages multiple jsproj files.

Tim Barham added 3 commits January 28, 2015 11:15
Adds support for `target`, `versions` and `arch` attributes on `<lib-file>` and `<framework>` elements in the windows platform of plugin.xml. This allows plugin authors to target different references to different target platforms.

Also adds support for `src` attribute as an alias for the `Include` attribute on the `<lib-file>` element (since `src` is documented, but `Include` is used by existing plugins).

Adds some tests to cover the new attributes. Updates existing plugin tests for windows8 platform to also test windows platform (left in windows8 tests to help verify backward compatibility with old windows8 platform).

As part of this change, refactored `jsproj` to `jsprojManager` to reflect the fact that, with the windows platform, this class now manages multiple jsproj files.

Note that I plan to rename some windows8 files and folders to windows, and jsproj.js to jsprojManager.js in a subsequent commit.
Renames `windows8` plugin platform folders in tests to `windows`. Renames `jsproj.js` to `jsprojManager.js`.
Renames `windows8.spec.js` to `windows.spec.js`.
@purplecabbage
Copy link
Contributor

Very cool Tim!

var frameworks = copyArray(valid_frameworks);

it('should write to correct project files when conditions are specified', function () {
var xpath = 'ProjectReference[@Include="' + dummyplugin + '\\src\\windows\\dummy1.vcxproj"][@Condition="\'$(Platform)\'==\'x64\'"]';
Copy link
Contributor

Choose a reason for hiding this comment

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

First failing test in Travis.

@purplecabbage
Copy link
Contributor

You should use path.join( ) in your tests when creating paths so the tests will still pass on MacOS, or check the environment and skip running your tests altogether.

@TimBarham
Copy link
Contributor Author

The tests use path.join(), but jsproj uses hard-coded backslashes (which is reasonable, because normally it would only run on Windows). This problem is showing up now because previously this code wasn’t exercised by tests.

I’m looking to see if it is feasible/reasonable to update jsproj to be platform agnostic, to the extent it needs to be for tests to pass. I’d prefer that to skipping the tests, so we get consistent coverage (the tests don’t build projects – just verify that files are modified as expected).

@purplecabbage
Copy link
Contributor

yeah, whichever approach is easier. Ultimately the test in this case is invalid, but we don't want it appearing to break the build.

Tim Barham and others added 3 commits February 4, 2015 18:30
@TimBarham
Copy link
Contributor Author

I'll close this PR, and open a new, cleaned up PR.

@TimBarham TimBarham closed this Feb 11, 2015
@TimBarham TimBarham deleted the CB-8123-final branch February 16, 2015 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants