Skip to content

Conversation

petecheslock
Copy link
Contributor

@petecheslock petecheslock commented Jan 22, 2024

Taking a first pass of this - see the issue here: getappmap/appmap-js#1555. AppMap spawns separate index files for every appmap.yaml - so excluding the test data from the package.

Fixes #272

pyproject.toml Outdated
exclude = ['_appmap/wrapt']
exclude = [
'_appmap/wrapt',
'_appmap/test/data/*/appmap.yml'
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe all of _appmap/test would be fine?

@apotterri
Copy link
Contributor

It's important to ensure that all the files necessary to run the tests are included in the source distribution (sdist) that gets created by poetry (and uploaded to the PyPI).

If they are, then this change is fine, and in fact can be improved by Kevin's suggestion of excluding all of _appmap/test.

If they're not included, though, you'll need to dig into the poetry doc and see if there's a different approach.

@apotterri
Copy link
Contributor

Also, the Travis build is red. Looks like you may need to pin pytest-django to an older version.

@kgilpin
Copy link
Contributor

kgilpin commented Jan 22, 2024

It's important to ensure that all the files necessary to run the tests are included in the source distribution (sdist) that gets created by poetry (and uploaded to the PyPI).

If they are, then this change is fine, and in fact can be improved by Kevin's suggestion of excluding all of _appmap/test.

If they're not included, though, you'll need to dig into the poetry doc and see if there's a different approach.

So there are different types of distributions? The package distribution and the source distribution?

@petecheslock
Copy link
Contributor Author

Thanks @apotterri i'll check out some docs to see what some ideas could be to solve this. I suppose if there isn't an easy way to exclude those files then the alternative solution would be for me to just modify the tests to use an appmap.yml with a different name and just change those filenames accordingly?

@kgilpin
Copy link
Contributor

kgilpin commented Jan 22, 2024

Thanks @apotterri i'll check out some docs to see what some ideas could be to solve this. I suppose if there isn't an easy way to exclude those files then the alternative solution would be for me to just modify the tests to use an appmap.yml with a different name and just change those filenames accordingly?

@petecheslock what I think he is saying is that there is more than one kind of Python distribution.

For example "package" distribution, and "source" distribution, which are similar, but have different purposes.

@apotterri please clarify

Renaming the appmap.yml files is a last resort if this can't be solved via packaging.

@petecheslock
Copy link
Contributor Author

Yea i think that is correct. I found this in the poetry project which gives an example of how i might exclude those files from the wheel distribution while also keeping them included in the source. python-poetry/poetry#3380 (comment)

going to give this a try and update this PR when i have it working.

@petecheslock
Copy link
Contributor Author

Ok - i think i have this working. With the recent change i do the following

poetry build

From there i get this:

Building appmap (1.18.1)
  - Building sdist
  - Built appmap-1.18.1.tar.gz
  - Building wheel
  - Built appmap-1.18.1-py3-none-any.whl

if i unzip the source tar.gz the _appmap/test directory exits

When i unzip the wheel - the test directory does NOT exist.

Then in my django oscar project i install the wheel from the local built version

$ head requirements.txt                                                                                                                                                                   
file:////Users/petecheslock/repos/appmap-python/dist/appmap-1.18.1-py3-none-any.whl

Then after install i checked the log and only one indexer is running (which is what is expected).

@apotterri
Copy link
Contributor

if i unzip the source tar.gz the _appmap/test directory exits

When i unzip the wheel - the test directory does NOT exist.

Yup, this is the desired behavior. 👍

@petecheslock
Copy link
Contributor Author

Thanks @apotterri - i updated the files (i thiink correctly) to use a working version on pytest-django and the tests now pass. Can you let me know if that is correct and then this is ready for review.

Copy link
Contributor

@apotterri apotterri left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for doing this.

@petecheslock petecheslock merged commit 3adcf00 into master Jan 22, 2024
@petecheslock petecheslock deleted the feat/exclude-appmap-test-files branch January 22, 2024 19:19
petecheslock added a commit that referenced this pull request Jan 22, 2024
* Exclude the appmap test files

* Update glob to exclude more files

* Include the necessary files only for the sdist exclude them otherwise

* Use a lower working version of pytest-django

* Update the tox config correctly
petecheslock added a commit that referenced this pull request Jan 22, 2024
* Exclude the appmap test files

* Update glob to exclude more files

* Include the necessary files only for the sdist exclude them otherwise

* Use a lower working version of pytest-django

* Update the tox config correctly
appland-release pushed a commit that referenced this pull request Jan 22, 2024
## [1.18.2](v1.18.1...v1.18.2) (2024-01-22)

### Bug Fixes

* Exclude the appmap test files ([#271](#271)) ([9ff91b8](9ff91b8))
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.

appmap.yml files are not shipped in releases

3 participants