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

make-rules: use 'pkgdepend resolve -e' and REQUIRED_PACKAGES #3883

Closed
wants to merge 3 commits into from

Conversation

@richlowe
Copy link
Contributor

richlowe commented Jan 18, 2018

The pkg(5) in OI is new enough now to allow the use of 'pkgdepend
resolve -e' to vastly reduce build times at the expense of manually
maintaining a list of dependencies.

This change allows that to happen in the "new" way, using the
REQUIRED_PACKAGES list that most Makefiles already contain, rather than
the "old" way way, using a static resolve.deps which was never actively
used in OI.

The pkg(5) in OI is new enough now to allow the use of 'pkgdepend
resolve -e' to vastly reduce build times at the expense of manually
maintaining a list of dependencies.

This change allows that to happen in the "new" way, using the
REQUIRED_PACKAGES list that most Makefiles already contain, rather than
the "old" way way, using a static resolve.deps which was never actively
used in OI.
@richlowe
Copy link
Contributor Author

richlowe commented Jan 18, 2018

I haven't updated any existing packages with static dependencies in *.p5m, nor have I checked whether the REQUIRED_PACKAGES lists already in most packages are complete or appropriate for OI.

I have a vague feeling this could cause chaos when first used.

It does, however, speed the build up by several orders of magnitude in correct components.
The meat of the ips.mk change is from upstream circa 2014

@pyhalov
Copy link
Contributor

pyhalov commented Jan 18, 2018

This looks fine, I like this.
I have 2 questions:

  1. can we avoid using -e flag while doing 'gmake REQUIRED_PACKAGES' ? (we already know the list is incomplete, if we do this)
  2. we need mass rebuild to find out if anything is broken... @alarcher - can you assist in this? (no need, so far it's evident that a lot is broken)
@pyhalov
Copy link
Contributor

pyhalov commented Jan 18, 2018

  1. This breaks components which don't have REQUIRED_PACKAGES set. And this is a rather big set of packages....
    suggestions:
  1. somehow find out if REQUIRED_PACKAGES set in component's Makefile. If they are set, use -e (well, we have some packages, which don't set REQUIRED_PACKAGES, just because it's system/library, but I think it's not a big set).
  2. fix REQUIRED_PACKAGES for all components :)

Another issue I see. Sometimes component contains REQUIRED_PACKAGES, which include component-provided packages. We usually exclude them, so that one can get all build requirements in case, when current component is still not built. This change will break such components (e.g. database/mariadb-101). Again, either we somehow fix this logic or add all dependencies to RQUIRED_PACKAGES....

@pyhalov
Copy link
Contributor

pyhalov commented Jan 18, 2018

@alarcher, @xen0l - can we populate REQUIRED_PACKAGES together with gcc-6 migration? @alarcher - as you rebuild all the packages anyway, can you add this change to your branch? Well, it'can make a list of possible conflicts much more :)

@pyhalov pyhalov added the needs_work label Jan 18, 2018
@richlowe
Copy link
Contributor Author

richlowe commented Jan 18, 2018

We can't tell whether REQUIRED_PACKAGES is set, because it defaults to some (small) subset anyway. So it's always set to something, but probably something incomplete.

I think I would suggest a global run of gmake REQUIRED_PACKAGES as well as the full rebuild, when testing? If not, see also my answer regarding that. Nulling RESOLVE_DEPS in the problem makefiles should make this whole thing a no-op.

I think self-referential REQUIRED_PACKAGES should be ok. Have you seen problems?

emptying RESOLVE_DEPS, as we do, should make gmake REQUIRED_PACKAGES do the right thing. Are you having problems?

@pyhalov
Copy link
Contributor

pyhalov commented Jan 18, 2018

OK, so a) gmake REQUIRED_PACKAGES work as expected, b) we need to do mass gmake REQUIRED_PACKAGES and to set 'RESOLVE_DEPS=' in components with inter-packages dependencies...

@alarcher
Copy link
Contributor

alarcher commented Jan 18, 2018

@richlowe @pyhalov I can cherry-pick this change to the gcc-6 build zone. During the gcc-6/gcc-7 rebuilds a number of components generated an empty REQUIRED_PACKAGES and would require manual intervention.
Let me know if I should run the suggested global gmake REQUIRED_PACKAGES and report the results. FYI a full rebuild would take a night on my machine.
Self-referential REQUIRED_PACKAGES cause issues for a build from scratch excluding install from an external repository like I did for gcc-6 (I started from a minimal install + build-essential).

@pyhalov
Copy link
Contributor

pyhalov commented Jan 18, 2018

I think we want auto-generated REQUIRED_PACKAGES added for components, which currently miss them together with gcc-6 changeset. However, we'll have to deal somehow with self-referential REQUIRED_PACKAGES. For this manual work will be necessary - identify all such components, fix REQUIRED_PACKAGES and add 'RESOLVE_DEPS='. I thought about setting RESOLVE_DEPS to REQUIRE_PACKAGES + auto-extracted package fmris, but I'm not sure it would be straightforward to extract them, for example, from -PYV manifests.

@pyhalov
Copy link
Contributor

pyhalov commented Jan 18, 2018

However... RESOLVE_DEPS are necessary after $(MANIFEST_BASE)-%.mogrified target was finished. After this we can just use transform to extract pkg.fmris from manifests just like REQUIRED_PACKAGES does for dependencies.

@pyhalov pyhalov force-pushed the richlowe:fast-resolve-deps branch from 83e6de1 to 74d8898 Jan 19, 2018
@normjacobs
Copy link
Contributor

normjacobs commented Jan 19, 2018

@richlowe
Copy link
Contributor Author

richlowe commented Jan 19, 2018

Yes, it needs a full run of make REQUIRED_PACKAGES (or a hack I put together that pulls it together from the already built packages), and a full build to test it.

@pyhalov
Copy link
Contributor

pyhalov commented Feb 10, 2018

Was merged as part of #3014

@pyhalov pyhalov closed this Feb 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.