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

Comp/package component refactoring #5504

Conversation

theobat
Copy link
Contributor

@theobat theobat commented Mar 10, 2021

So this a work in progress, I'm stuck because I don't know how to get the proper set of components a local package should build (current behavior is, I try to ubild every component, which probably cause some of the errors in the tests).

@qrilka this is the PR I mentioned earlier.

Maybe you can review this (once the other doc PR is merged) and give me some feedback (it's still very messy)

@theobat theobat force-pushed the comp/package-component-refactoring branch from 4a7507c to b1e7f2b Compare March 11, 2021 08:58
@qrilka
Copy link
Contributor

qrilka commented Mar 14, 2021

@theobat the diff seems to contain #5464 - would you mind rebasing?

@theobat
Copy link
Contributor Author

theobat commented Mar 14, 2021

yeah sorry I thought I already did it.
It's done, I believe the main issues are :

  • I don't know how to use the ghc-registered internal-libraries components in DumpPackage which certainly cause problems.
  • I don't know what component set I should build for the LocalPackage (taking all of them is the current behavior, but it's obviously lousy)

Otherwise as far as the changes are concerned, if you filter the mess, it's mostly about adding NamedComponent everywhere in ConstructPlan and Build.Execute. In particular, the datatype Package is changed quite significantly, to register dependencies by components (All the dependencies where mixed before).

@theobat theobat force-pushed the comp/package-component-refactoring branch from b1e7f2b to 434d134 Compare March 14, 2021 17:41
theobat added 4 commits March 14, 2021 18:41

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@qrilka
Copy link
Contributor

qrilka commented Mar 14, 2021

Not sure about internal libraries and getting installed components info is one of questions that need to be resolved if we keep the way Stack currently works.

As for components - by default all of them should be included but filtered out by build flags which set whether tests/benches are to be included.

Hope this helps at least a bit.

I'll start reviewing this tomorrow

@qrilka
Copy link
Contributor

qrilka commented Mar 16, 2021

@theobat I had some (not very thorough) overview of this PR and I'm not sure what kind of a feedback do you want. In the code I don't see any obvious problems (but I had rather high-level overview of the diff) and tests are surely failing as GitHub shows. Maybe you have some concrete questions I could help finding an answer for? Do you have a plan how to continue here? If I was attacking this I would start with some MVP, e.g. doing a very minimal example with only base as a dependency but also involving components and then increase number of features used as you progress.

@qrilka
Copy link
Contributor

qrilka commented Apr 12, 2021

@theobat are you still working on this?

@theobat
Copy link
Contributor Author

theobat commented Apr 12, 2021

@qrilka I'll admit I don't have a lot of time, I wish I did, so I guess my answer is "not right now, and probably not enough in the coming weeks/months". The other significant issue is stack does not build on my mac anymore which does hinder the whole process (I also have ubuntu but it's not my day to day work computer so it's less convenient).
Finally I don't know what's wrong with my changes, I only added a component name to the build process, so I'll have to look at the tests one by one to see what's wrong. I suspect it's related to inconsistent flags between component based builds and tests but it's hand wavy...

@qrilka
Copy link
Contributor

qrilka commented Apr 18, 2021

It looks like I have also not that much time, I thought about trying out your branch but didn't find time yet. As about mac - that's surely not something I could help with. If you have any ideas about immediate tasks here I could look into - please share, probably that could make it more easy to find some time if it were something more concrete.

@theobat theobat force-pushed the comp/package-component-refactoring branch from 434d134 to 527a5ae Compare April 22, 2021 16:40
@theobat
Copy link
Contributor Author

theobat commented Apr 22, 2021

So, I managed to fix the issue with cocoa on mac thanks to this : purescript/purescript#3960 (comment)

I'll try to look at the failing tests in the coming weeks

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