Skip to content

Commit a295be1

Browse files
indygregindygreg
committed
pyoxidizer: log the actual resource collector behavior
See the changelog entry. Before, log messages were deceptive and could be wrong. After, the collector returns what actions it performed and we report these actions to the end-user. There is infinite potential to improve the level of detail here. But we start simple by recording basic resource operations such as ignore and add.
1 parent bebbaab commit a295be1

7 files changed

Lines changed: 278 additions & 135 deletions

File tree

‎pyoxidizer/docs/pyoxidizer_history.rst‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,15 @@ New Features
9595
https://github.com/indygreg/python-build-standalone/releases/tag/20211012 and
9696
https://github.com/indygreg/python-build-standalone/releases/tag/20211017 for
9797
the full list of changes.
98+
* When in verbose mode, messages will be printed displaying the actual result
99+
of every request to add a resource. Before, the Starlark code would emit a
100+
message like ``adding extension module foo`` before requesting the addition
101+
and the operation could no-op. This behavior was misleading and hard to debug
102+
since it often implied a resource was added when in fact it wasn't! The new
103+
behavior is for the resource collector to tell its callers exactly what
104+
actions it took and for the actual results to be displayed to the end-user.
105+
This should hopefully make it easier to debug issues with how resources are
106+
added to binaries.
98107

99108
Other Relevant Changes
100109
^^^^^^^^^^^^^^^^^^^^^^

‎pyoxidizer/src/py_packaging/binary.rs‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@ use {
2222
PythonPackageResource, PythonResource,
2323
},
2424
resource_collection::{
25-
CompiledResourcesCollection, PrePackagedResource, PythonResourceAddCollectionContext,
25+
AddResourceAction, CompiledResourcesCollection, PrePackagedResource,
26+
PythonResourceAddCollectionContext,
2627
},
2728
},
2829
std::{
@@ -361,7 +362,7 @@ pub trait PythonBinaryBuilder {
361362
fn add_distribution_resources(
362363
&mut self,
363364
callback: Option<ResourceAddCollectionContextCallback>,
364-
) -> Result<()>;
365+
) -> Result<Vec<AddResourceAction>>;
365366

366367
/// Add a `PythonModuleSource` to the resources collection.
367368
///
@@ -372,7 +373,7 @@ pub trait PythonBinaryBuilder {
372373
&mut self,
373374
module: &PythonModuleSource,
374375
add_context: Option<PythonResourceAddCollectionContext>,
375-
) -> Result<()>;
376+
) -> Result<Vec<AddResourceAction>>;
376377

377378
/// Add a `PythonPackageResource` to the resources collection.
378379
///
@@ -383,7 +384,7 @@ pub trait PythonBinaryBuilder {
383384
&mut self,
384385
resource: &PythonPackageResource,
385386
add_context: Option<PythonResourceAddCollectionContext>,
386-
) -> Result<()>;
387+
) -> Result<Vec<AddResourceAction>>;
387388

388389
/// Add a `PythonPackageDistributionResource` to the resources collection.
389390
///
@@ -394,7 +395,7 @@ pub trait PythonBinaryBuilder {
394395
&mut self,
395396
resource: &PythonPackageDistributionResource,
396397
add_context: Option<PythonResourceAddCollectionContext>,
397-
) -> Result<()>;
398+
) -> Result<Vec<AddResourceAction>>;
398399

399400
/// Add a `PythonExtensionModule` to make available.
400401
///
@@ -408,14 +409,14 @@ pub trait PythonBinaryBuilder {
408409
&mut self,
409410
extension_module: &PythonExtensionModule,
410411
add_context: Option<PythonResourceAddCollectionContext>,
411-
) -> Result<()>;
412+
) -> Result<Vec<AddResourceAction>>;
412413

413414
/// Add a `File` to the resource collection.
414415
fn add_file_data(
415416
&mut self,
416417
file: &File,
417418
add_context: Option<PythonResourceAddCollectionContext>,
418-
) -> Result<()>;
419+
) -> Result<Vec<AddResourceAction>>;
419420

420421
/// Filter embedded resources against names in files.
421422
///

‎pyoxidizer/src/py_packaging/standalone_builder.rs‎

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,8 @@ use {
3535
PythonPackageResource, PythonResource,
3636
},
3737
resource_collection::{
38-
PrePackagedResource, PythonResourceAddCollectionContext, PythonResourceCollector,
38+
AddResourceAction, PrePackagedResource, PythonResourceAddCollectionContext,
39+
PythonResourceCollector,
3940
},
4041
},
4142
slog::warn,
@@ -657,7 +658,9 @@ impl PythonBinaryBuilder for StandalonePythonExecutableBuilder {
657658
fn add_distribution_resources(
658659
&mut self,
659660
callback: Option<ResourceAddCollectionContextCallback>,
660-
) -> Result<()> {
661+
) -> Result<Vec<AddResourceAction>> {
662+
let mut actions = vec![];
663+
661664
let core_component = self
662665
.target_distribution
663666
.core_license
@@ -686,7 +689,7 @@ impl PythonBinaryBuilder for StandalonePythonExecutableBuilder {
686689
.add_licensed_component(component.clone())?;
687690
}
688691

689-
self.add_python_extension_module(&ext, Some(add_context))?;
692+
actions.extend(self.add_python_extension_module(&ext, Some(add_context))?);
690693
}
691694

692695
for resource in self
@@ -725,23 +728,23 @@ impl PythonBinaryBuilder for StandalonePythonExecutableBuilder {
725728
component.set_flavor(ComponentFlavor::PythonPackage);
726729

727730
self.resources_collector.add_licensed_component(component)?;
728-
self.add_python_module_source(source, Some(add_context))?;
731+
actions.extend(self.add_python_module_source(source, Some(add_context))?);
729732
}
730733
PythonResource::PackageResource(r) => {
731-
self.add_python_package_resource(r, Some(add_context))?;
734+
actions.extend(self.add_python_package_resource(r, Some(add_context))?);
732735
}
733736
_ => panic!("should not get here since resources should be filtered above"),
734737
}
735738
}
736739

737-
Ok(())
740+
Ok(actions)
738741
}
739742

740743
fn add_python_module_source(
741744
&mut self,
742745
module: &PythonModuleSource,
743746
add_context: Option<PythonResourceAddCollectionContext>,
744-
) -> Result<()> {
747+
) -> Result<Vec<AddResourceAction>> {
745748
let add_context = add_context.unwrap_or_else(|| {
746749
self.packaging_policy
747750
.derive_add_collection_context(&module.into())
@@ -755,7 +758,7 @@ impl PythonBinaryBuilder for StandalonePythonExecutableBuilder {
755758
&mut self,
756759
resource: &PythonPackageResource,
757760
add_context: Option<PythonResourceAddCollectionContext>,
758-
) -> Result<()> {
761+
) -> Result<Vec<AddResourceAction>> {
759762
let add_context = add_context.unwrap_or_else(|| {
760763
self.packaging_policy
761764
.derive_add_collection_context(&resource.into())
@@ -769,7 +772,7 @@ impl PythonBinaryBuilder for StandalonePythonExecutableBuilder {
769772
&mut self,
770773
resource: &PythonPackageDistributionResource,
771774
add_context: Option<PythonResourceAddCollectionContext>,
772-
) -> Result<()> {
775+
) -> Result<Vec<AddResourceAction>> {
773776
let add_context = add_context.unwrap_or_else(|| {
774777
self.packaging_policy
775778
.derive_add_collection_context(&resource.into())
@@ -783,16 +786,17 @@ impl PythonBinaryBuilder for StandalonePythonExecutableBuilder {
783786
&mut self,
784787
extension_module: &PythonExtensionModule,
785788
add_context: Option<PythonResourceAddCollectionContext>,
786-
) -> Result<()> {
789+
) -> Result<Vec<AddResourceAction>> {
787790
let add_context = add_context.unwrap_or_else(|| {
788791
self.packaging_policy
789792
.derive_add_collection_context(&extension_module.into())
790793
});
791794

792-
if let Some(mut build_context) = self
795+
let (actions, build_context) = self
793796
.resources_collector
794-
.add_python_extension_module_with_context(extension_module, &add_context)?
795-
{
797+
.add_python_extension_module_with_context(extension_module, &add_context)?;
798+
799+
if let Some(mut build_context) = build_context {
796800
// Resources collector doesn't doesn't know about ignored libraries. So filter
797801
// them here.
798802
build_context.static_libraries = build_context
@@ -816,14 +820,14 @@ impl PythonBinaryBuilder for StandalonePythonExecutableBuilder {
816820
.insert(extension_module.name.clone(), build_context);
817821
}
818822

819-
Ok(())
823+
Ok(actions)
820824
}
821825

822826
fn add_file_data(
823827
&mut self,
824828
file: &File,
825829
add_context: Option<PythonResourceAddCollectionContext>,
826-
) -> Result<()> {
830+
) -> Result<Vec<AddResourceAction>> {
827831
let add_context = add_context.unwrap_or_else(|| {
828832
self.packaging_policy
829833
.derive_add_collection_context(&file.into())

‎pyoxidizer/src/starlark/python_distribution.rs‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ use {
2222
policy::PythonPackagingPolicy, resource::PythonResource,
2323
resource_collection::PythonResourceAddCollectionContext,
2424
},
25+
slog::info,
2526
starlark::{
2627
environment::TypeValues,
2728
eval::call_stack::CallStack,
@@ -380,15 +381,18 @@ impl PythonDistributionValue {
380381
},
381382
);
382383

383-
builder
384+
for action in builder
384385
.add_distribution_resources(Some(callback))
385386
.map_err(|e| {
386387
ValueError::from(RuntimeError {
387388
code: "PYOXIDIZER_BUILD",
388389
message: format!("{:?}", e),
389390
label: "to_python_executable()".to_string(),
390391
})
391-
})?;
392+
})?
393+
{
394+
info!(pyoxidizer_context.logger(), "{}", action.to_string());
395+
}
392396

393397
Ok(Value::new(PythonExecutableValue::new(builder, policy)))
394398
}

‎pyoxidizer/src/starlark/python_executable.rs‎

Lines changed: 40 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -632,16 +632,17 @@ impl PythonExecutableValue {
632632
) -> ValueResult {
633633
let inner = module.inner(label)?;
634634

635-
info!(
636-
context.logger(),
637-
"adding Python source module {}", inner.m.name;
638-
);
639-
640635
let mut exe = self.inner(label)?;
641636

642637
error_context(label, || {
643-
exe.add_python_module_source(&inner.m, inner.add_context.clone())
644-
.with_context(|| format!("adding {}", module.to_repr()))
638+
for action in exe
639+
.add_python_module_source(&inner.m, inner.add_context.clone())
640+
.with_context(|| format!("adding {}", module.to_repr()))?
641+
{
642+
info!(context.logger(), "{}", action.to_string());
643+
}
644+
645+
Ok(())
645646
})?;
646647

647648
Ok(Value::new(NoneType::None))
@@ -655,17 +656,17 @@ impl PythonExecutableValue {
655656
) -> ValueResult {
656657
let inner = resource.inner(label)?;
657658

658-
info!(
659-
context.logger(),
660-
"adding Python package resource {}",
661-
inner.r.symbolic_name()
662-
);
663-
664659
let mut exe = self.inner(label)?;
665660

666661
error_context(label, || {
667-
exe.add_python_package_resource(&inner.r, inner.add_context.clone())
668-
.with_context(|| format!("adding {}", resource.to_repr()))
662+
for action in exe
663+
.add_python_package_resource(&inner.r, inner.add_context.clone())
664+
.with_context(|| format!("adding {}", resource.to_repr()))?
665+
{
666+
info!(context.logger(), "{}", action.to_string());
667+
}
668+
669+
Ok(())
669670
})?;
670671

671672
Ok(Value::new(NoneType::None))
@@ -679,16 +680,17 @@ impl PythonExecutableValue {
679680
) -> ValueResult {
680681
let inner = resource.inner(label)?;
681682

682-
info!(
683-
context.logger(),
684-
"adding package distribution resource {}:{}", inner.r.package, inner.r.name
685-
);
686-
687683
let mut exe = self.inner(label)?;
688684

689685
error_context(label, || {
690-
exe.add_python_package_distribution_resource(&inner.r, inner.add_context.clone())
691-
.with_context(|| format!("adding {}", resource.to_repr()))
686+
for action in exe
687+
.add_python_package_distribution_resource(&inner.r, inner.add_context.clone())
688+
.with_context(|| format!("adding {}", resource.to_repr()))?
689+
{
690+
info!(context.logger(), "{}", action.to_string());
691+
}
692+
693+
Ok(())
692694
})?;
693695

694696
Ok(Value::new(NoneType::None))
@@ -702,16 +704,17 @@ impl PythonExecutableValue {
702704
) -> ValueResult {
703705
let inner = module.inner(label)?;
704706

705-
info!(
706-
context.logger(),
707-
"adding extension module {}", inner.em.name
708-
);
709-
710707
let mut exe = self.inner(label)?;
711708

712709
error_context(label, || {
713-
exe.add_python_extension_module(&inner.em, inner.add_context.clone())
714-
.with_context(|| format!("adding {}", module.to_repr()))
710+
for action in exe
711+
.add_python_extension_module(&inner.em, inner.add_context.clone())
712+
.with_context(|| format!("adding {}", module.to_repr()))?
713+
{
714+
info!(context.logger(), "{}", action.to_string());
715+
}
716+
717+
Ok(())
715718
})?;
716719

717720
Ok(Value::new(NoneType::None))
@@ -725,17 +728,17 @@ impl PythonExecutableValue {
725728
) -> ValueResult {
726729
let inner = file.inner(label)?;
727730

728-
info!(
729-
context.logger(),
730-
"adding file data {}",
731-
inner.file.path().display()
732-
);
733-
734731
let mut exe = self.inner(label)?;
735732

736733
error_context(label, || {
737-
exe.add_file_data(&inner.file, inner.add_context.clone())
738-
.with_context(|| format!("adding {}", file.to_repr()))
734+
for action in exe
735+
.add_file_data(&inner.file, inner.add_context.clone())
736+
.with_context(|| format!("adding {}", file.to_repr()))?
737+
{
738+
info!(context.logger(), "{}", action.to_string());
739+
}
740+
741+
Ok(())
739742
})?;
740743

741744
Ok(Value::new(NoneType::None))

0 commit comments

Comments
 (0)