Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Fix VTK duplicate linking libraries #10518

Closed
wants to merge 1 commit into from
Closed

Fix VTK duplicate linking libraries #10518

wants to merge 1 commit into from

Conversation

larshg
Copy link
Contributor

@larshg larshg commented Mar 23, 2020 •
edited
Loading

Describe the pull request
Fixes VTK to not mix debug and release 3. party libraries, however they are still added two times, but it doesn't seem to cause trouble. Guess the linker can identify its already added or?
I propose this for now as it fixes the immediate problem, where the other is more nice to have. And soon 9.0 will probably take over soon -> #9960.

Probably fixes this : #10314

  • Which triplets are supported/not supported? Have you updated the CI baseline?
    Should be for all non-static triplets. Haven't updated the CI baseline, no.

  • Does your PR follow the maintainer guide?
    Should do so yes.

@msftclas
Copy link

msftclas commented Mar 23, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

@ArashPartow ArashPartow mentioned this pull request Mar 23, 2020
Closed
@larshg
Copy link
Contributor Author

larshg commented Mar 24, 2020 •
edited
Loading

I'll have to update the patch again.. will do it asap

Should be correct now.

NancyLi1013
NancyLi1013 suggested changes Mar 24, 2020
View reviewed changes
@larshg larshg requested a review from NancyLi1013 March 25, 2020 21:41
@NancyLi1013
Copy link
Contributor

Need to test features.

@NancyLi1013
Copy link
Contributor

/azp run

@larshg
Copy link
Contributor Author

larshg commented Mar 26, 2020

Need to test features.

Was it a note to self or something I should/can do :) ?

@NancyLi1013
Copy link
Contributor

If possible, you can test them too.

@JackBoosY JackBoosY changed the title Fix VTK dublicate linking libraries Fix VTK duplicate linking libraries Apr 3, 2020
@larshg larshg mentioned this pull request Apr 6, 2020
Closed
@NancyLi1013
Copy link
Contributor

@larshg
Sorry for the late relpy.
since icu requires python3 to build, which blocks this PR to be tested.
I will test these features once PR #10656 merged.

@NancyLi1013 NancyLi1013 added the requires:testing Needs tests added before merging label Apr 17, 2020
@larshg
Copy link
Contributor Author

larshg commented Apr 20, 2020

Rebased.

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@larshg
Copy link
Contributor Author

larshg commented Apr 22, 2020

Raised version to 14 after the merge of the other VTK PR.

@larshg larshg mentioned this pull request Apr 22, 2020
Closed
@larshg
Copy link
Contributor Author

larshg commented May 10, 2020

How can one go about "testing" this PR?

Is it "just" to build vtk with all features enabled?

@NancyLi1013
Copy link
Contributor

Hi @larshg

Yes, you're right. All features are needed to be tested manually by building vtk with them.

The testing was blocked by something before this. I continued to test this last week. Now the test is going well. I will update the test results today.

Sorry for the long time to wait.

@NancyLi1013
Copy link
Contributor

NancyLi1013 commented May 12, 2020 •
edited
Loading

Hi @larshg

Could you please resolve the conflicts?

All features have passed with the following triplets:

  • x86-windows
  • x64-windows-static
  • x64-windows

As for x64-linux, it was blocked by python3 in current machine. I need to test it on another Ubuntu machine with gcc 9.2.1. I will update the result later.

@larshg
Copy link
Contributor Author

larshg commented May 12, 2020

How is this possible now that VTK is upgraded to 9.0?

@NancyLi1013
Copy link
Contributor

VTK has been updated to 9.0.0 in PR #11148.
But what's problem about this?

@larshg
Copy link
Contributor Author

larshg commented May 13, 2020

The patch included here, is only for VTK 8.2. If I rebase I'll have to stick to the VTK 9.0 version and with its feature/dependencies.

Or else its downgraded to 8.2 again?

I'm not sure how to resolve the conflicts.

@NancyLi1013
Copy link
Contributor

Oh, sorry to hear that. I didn't notice this before.

But we still need to add a new patch for vtk 9.0. Since it has been merged to mater. It cannot be downgraded now.

@NancyLi1013
Copy link
Contributor

I will test the features on Linux platform after you resolve the conflicts.

@larshg
Copy link
Contributor Author

larshg commented May 13, 2020

I don't think it make sense to add this patch to 9.0 as they revamped their modules system / dependencies.

@larshg larshg closed this May 13, 2020
@NancyLi1013 NancyLi1013 removed the requires:testing Needs tests added before merging label May 14, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@NancyLi1013 NancyLi1013 NancyLi1013 approved these changes

Assignees

@NancyLi1013 NancyLi1013

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@larshg @msftclas @NancyLi1013