-
Notifications
You must be signed in to change notification settings - Fork 81
Allow make commands to be run on a single project #110
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
base: main
Are you sure you want to change the base?
Conversation
6b81c72 to
67b613a
Compare
| # These are the directories that will be built | ||
| DIRS := $(wildcard $(SOURCE_FOLDER)/$(project)) | ||
| SUBDIRS := $(filter-out $(EXCLUDED_PROJECT_PATHS), $(DIRS)) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just keep it simple with
SUBDIRS := $(filter-out src/dbtools-mcp-server src/mysql-mcp-server src/oci-pricing-mcp-server src/oracle-db-doc-mcp-server,$(wildcard src/*))
SUBDIRS ?= $(filter-out src/dbtools-mcp-server src/mysql-mcp-server src/oci-pricing-mcp-server src/oracle-db-doc-mcp-server,$(wildcard src/*))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can then run the commands with the following
SUBDIRS=src/oci-api-mcp-server make build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The complexity here possibly seems a bit overkill, but it's also been about 16 years since I've used Make, so I can't really say much meaningful in terms of what I would suggest to make it simpler.
However, I would prefer something even more simple for consumers, like make project=compute build and have the other parts added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the benefit to @cboffa13's approach here is you dont have to write src/ on the start of the project you wanna build. Since the src variable was defined at the top of the Makefile to make that happen, he decided to reuse it with the other excluded_projects.
| DIRS := $(wildcard $(SOURCE_FOLDER)/$(project)) | ||
| SUBDIRS := $(filter-out $(EXCLUDED_PROJECT_PATHS), $(DIRS)) | ||
|
|
||
| .PHONY: test format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one issue, though. If you were to run the test target using a project such as oci-cloud-guard-mcp-server where the individual coverage is less than the overall coverage figure of 69, the result is a failure, where it is actually not a failure.
$ make project=oci-cloud-guard-mcp-server test
Testing src/oci-cloud-guard-mcp-server
warning: `VIRTUAL_ENV=/Users/irahmed/development/projects/oci-console/fork-oracle-mcp/.venv` does not match the project environment path `.venv` and will be ignored; use `--active` to target the active environment instead
========================================== test session starts ===========================================
platform darwin -- Python 3.13.7, pytest-9.0.2, pluggy-1.6.0
rootdir: /Users/irahmed/development/projects/oci-console/oracle-mcp/src/oci-cloud-guard-mcp-server
configfile: pyproject.toml
plugins: anyio-4.12.0, asyncio-1.3.0, cov-7.0.0
asyncio: mode=Mode.STRICT, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collected 7 items
oracle/oci_cloud_guard_mcp_server/tests/test_cloud_guard_tools.py ....... [100%]
============================================ warnings summary ============================================
.venv/lib/python3.13/site-packages/pydantic/json_schema.py:2442
/Users/irahmed/development/projects/oci-console/oracle-mcp/src/oci-cloud-guard-mcp-server/.venv/lib/python3.13/site-packages/pydantic/json_schema.py:2442: PydanticJsonSchemaWarning: Default value typing.Literal['OPEN', 'RESOLVED', 'DISMISSED', 'DELETED', 'UNKNOWN_ENUM_VALUE'] is not JSON serializable; excluding default from JSON schema [non-serializable-default]
warnings.warn(message, PydanticJsonSchemaWarning)
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
============================================= tests coverage =============================================
____________________________ coverage: platform darwin, python 3.13.7-final-0 ____________________________
Name Stmts Miss Branch BrPart Cover Missing
------------------------------------------------------------------------------------------
oracle/oci_cloud_guard_mcp_server/models.py 72 22 14 1 59.30% 16-28, 167-169, 180-183, 243-245
oracle/oci_cloud_guard_mcp_server/server.py 59 13 12 4 76.06% 29-42, 77, 78->80, 81, 141
------------------------------------------------------------------------------------------
TOTAL 131 35 26 5 66.88%
Coverage HTML written to dir htmlcov
Required test coverage of 64.8% reached. Total coverage: 66.88%
====================================== 7 passed, 1 warning in 1.77s ======================================
/Library/Developer/CommandLineTools/usr/bin/make combine-coverage
uv run coverage combine
Combined data file .coverage.oci-cloud-guard-mcp-server
uv run coverage html
Wrote HTML report to htmlcov/index.html
uv run coverage report --fail-under=69
Name Stmts Miss Branch BrPart Cover
--------------------------------------------------------------------------------------------------------------
src/oci-cloud-guard-mcp-server/oracle/oci_cloud_guard_mcp_server/models.py 72 22 14 1 59%
src/oci-cloud-guard-mcp-server/oracle/oci_cloud_guard_mcp_server/server.py 59 13 12 4 76%
--------------------------------------------------------------------------------------------------------------
TOTAL 131 35 26 5 67%
Coverage failure: total of 67 is less than fail-under=69
make[1]: *** [combine-coverage] Error 2
make: *** [test] Error 2There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this is because we are combining the code coverage of all the servers. tbh i think this is a good thing and will force us to meet a minimum coverage bar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe if project is set, we should not combine. The failure above is a false positive. Users using this will see a failure and may start debugging it when it is not a failure, actually. That combined figure of 69 is for when we run test against all the MCP servers.
Description
As the project grows it takes longer to build and install all the packages when running commands like
make buildormake install. This PR lets us run commands for a single project likemake project=project_name buildso other teams dont need to wait for other projects to build if they are only working on a specific server.For example:
make build- runs the build command on all the folders in thesrcfolder besides the excluded packagesmake project=oci-compute-mcp-server buildbuilds just the compute mcp server and nothing elseFixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Checklist: